Skip to content
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

Implement config file support #223

Merged
merged 1 commit into from
Nov 15, 2022
Merged

Implement config file support #223

merged 1 commit into from
Nov 15, 2022

Conversation

bmish
Copy link
Owner

@bmish bmish commented Nov 13, 2022

Fixes #95.

README documentation located here.

TODO:

  • Add tests
  • Use a JSON schema (ajv) to validate config file
  • Fix boolean options
  • Throw error when duplicate --config-emoji

@bmish bmish added the enhancement New feature or request label Nov 13, 2022
@bmish bmish force-pushed the config-file branch 3 times, most recently from eb2c917 to 63fa0ae Compare November 14, 2022 16:35
README.md Show resolved Hide resolved
@bmish bmish force-pushed the config-file branch 2 times, most recently from d50c8fc to aa54b4e Compare November 14, 2022 17:40
Comment on lines +55 to +56
// TODO: use TypeScript 4.9 satisfies keyword ([key in OPTION_TYPE]...) to ensure all options are included without losing type information.
export const OPTION_DEFAULTS = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Record instead. You will get type errors if you the values aren't in sync with OPTION_TYPE

Suggested change
// TODO: use TypeScript 4.9 satisfies keyword ([key in OPTION_TYPE]...) to ensure all options are included without losing type information.
export const OPTION_DEFAULTS = {
export const OPTION_DEFAULTS: Record<OPTION_TYPE, string> = {

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, just tried it and it looks like it's equivalent to the type I was originally trying to use { [key in OPTION_TYPE]: string | string[] | boolean | undefined } and has the same problem. While it does correctly ensure all options are included in the object, it loses type information since accessing a single option from the object now could have any of those possible types instead of what should be a known type. The new satisfies feature in TypeScript 4.9 looks like it will solve this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on why you need the exact type for each of them?
It doesn't seem like it gives extra value 🤔

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's required actually. This object stores default values for each config option. These default values are used throughout the codebase. When they have inexact types (i.e. could be one of several types), they won't be able to be used easily due to type mismatches. For example, for an option with an array default value, we won't be able to handle/iterate the default value properly if the type says it could also be a string, boolean, etc.

@bmish bmish marked this pull request as ready for review November 15, 2022 02:31
@bmish bmish merged commit 348970e into main Nov 15, 2022
@bmish bmish deleted the config-file branch November 15, 2022 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow configuration in config file or package.json object
3 participants