-
-
Notifications
You must be signed in to change notification settings - Fork 287
feat: consolidate dendron configs #1295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Left some minor comments but go ahead with this. For this commit, is the plan to carry out phase I of config migration? Can we have this ready to be merged for next week's release?
For deprecation notice, since we'll do automatic migration, we might not need to do any sort of deprecation notice besides announcing it in our release notes. FOr migrating to the new configuration, can we do it piecemeal?
Eg. Week1 -> migrate command config. Week2 -> migrate 2 more sections. Week3 -> migrate everything...
/** | ||
* Constants holding all command config related {@link DendronConfigEntry} | ||
*/ | ||
export const COMMANDS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thoughts between using constants vs enums? You can use Object.values
to get all enum values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values in the object are also constants that themselves holding other constants or functions that produce constants.
Are you suggesting we have nested enums (not sure if that's a thing)? or we flatten it out and have a giant enum that holds all possible labels and descriptions?
* @param value {@link dayOfWeekNumber} | ||
* @returns DendronConfigEntry | ||
*/ | ||
const FIRST_DAY_OF_WEEK = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a heads up that we're currently not supporting this option due to limitation in calendar widget. we'll add support back in eventually but FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am aware of this. I also have been taking note on other configs with some peculiarities that we need to address once we start actually migrating.
Yeah, I think it makes sense to announce what's going to change and then slowly migrate the configurations to the new one. I'll get it ready by next release, and also write up an announcement to add to the release notes of what the changes will entail. |
UPDATE: Please check the overview in the main body of this PR. Since this PR is waaaay too big, I go over the general structure of what is being added. This PR does not change any existing behavior of the config. It just adds the newly organized config modules. I will also be adding a document that goes over exactly which config is going to be mapped to which in a separate docs PR, which can be added to the upcoming release notes. |
Here is an exhaustive list of changes to every config key
Dendron Configurations
Site Configurations
|
Great job on putting in the ground work. Configuration renames look good, just a few changes suggested below
|
typescript supports initializing generics, would rewrite to the following in that case
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see config changes in conversation
* For config entries that can be an arbitrary value, only specify the label and description. | ||
* For config entries that have pre-defined choices, provide the value as well as label and description specific to that value. | ||
*/ | ||
export type DendronConfigEntry<T> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since value is optional we can make T=any
to cut down on boilerplate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL 😅 still so much to learn about ts.
feat: consolidate dendron configs
This PR consolidates and organizes all Dendron configurations.
Docs for config style guide: dendronhq/dendron-site#196
Docs for diff between as-is config and to-be config: TBD
for easier review, read in this order
packages/common-all/src/types/configs/base.ts
packages/common-all/src/types/configs/*
, starting frompackages/common-all/src/types/configs/dendronConfig.ts
packages/common-all/src/constants/configs/*
, starting formpackages/common-all/src/constants/configs/dendronConfig.ts
Overview of changes
The new configuration is organized into the following namespaces:
DendronGlobalConfig
)DendronPreviewConfig
)DendronPublishingConfig
)DendronWorkspaceConfig
)DendronCommandConfig
)DendronDevConfig
)Config type definitions
packages/common-all/src/types/configs/
along with a function that generates the defaults of those configs.DendronDevConfig
.Constants
packages/common-all/src/constants/configs/
DendronConfigEntry<T>
, where T is the type of the config value.initialValue
of the commandInsertNote
has aDendronConfigEntry<string>
like so:DendronConfigEntry
:T
is meaningless, but is there for completeness' sake.DendronConfigEntry
have a selected number of possible values, thevalue
is specified, andlabel
anddescription
is written to take account of the different behaviors respectivevalue
s enable.extract
of Note lookup looks like this:DendronConfigEntry
in multiple places. Some configurations benefit from dynamically generated descriptions. To reduce repeated code, there are functions that generateDendronConfigEntry
based on the value given.mermaid
syntax can happen in preview and publishing (as well as global). This is the function that generates the config entries.DendronConfigEntry
s.DendronConfigEntryCollection
.DendronConfigEntry
of it if you omit it.DendronConfigEntryCollection
looks like this:DendronConfigEntryCollection
(of a sub-namespace),Record
that maps possible config enum keys toDendronConfigEntry
NoteLookup
'sselection
mode.DendronConfigEntry
DendronConfigEntry
based on what value it is given.Conventions
Pull Request Checklist
You can go to dendron pull requests to see full details for items in this checklist.
General
Quality Assurance
Special Cases
Docs