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

Optional config #23

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Optional config #23

wants to merge 4 commits into from

Conversation

apoblock
Copy link

Support for optional configuration file, as described in #21
Please take a look @jwarby

@jwarby
Copy link
Owner

jwarby commented Dec 17, 2020

Hi, sorry but I didn't see the original issue (#21) that you raised.

Thanks for taking the time to implement it - I'll try and get to looking at this properly next week. Some initial thoughts after scanning through though:

  1. I think it would have been nice to use an existing config loading tool, eg something like cosmiconfig. It'd be one less responsibility for this package to consider, and I think it would give users some added flexibility too (eg if you are using 2 different types of views in your app, you could dump a config file into the main dir for each different type of views and whatever config library is used should pick those up and also merge with any options in config files higher up the tree, as you'd see with tools like ESLint, etc). Another thing (that I haven't checked properly so might be wrong) - I'm not sure if the init behaviour means we now have the default values for operation stored in multiple places or not - if so, we might want to consider moving the default values into their own file (or w/e) so that they can be easily reused.
  2. Just wondering about the bug fix to do with = and wondering if it might be better to address that in a separate PR so that it's easier to find reference to the change in the future?
  3. Looks like the branch originally came off the master branch because there are some release merge commits included - you might have to cherry-pick your work from a new branch created off develop.

Cheers!

@apoblock
Copy link
Author

Hi @jwarby, thanks for looking into this. I'm working on VSCode extension that uses this package, and having a config file will allow to use the same settings in the extension and in the npm task.

As for your points:

  1. I will look at cosmiconfig or rc - I wanted to keep it simple initially, and besides loading I wanted to be able to save configuration specified in CLI (when you specify other parameters with --init those will be saved in the generated config file)
  2. The PR for fix is pending here: Attribute parsing fix #22
  3. Will do

Cheers,
Andrew

@jwarby
Copy link
Owner

jwarby commented Dec 18, 2020

Cool, cheers @apoblock, will try to look at the PR for the fix next week too.

Re: the config saving option - it's cool, but to be entirely honest I'm not sure it's worth the extra complexity and added maintenance that it introduces, especially when you consider the fairly low number of CLI arguments that the project supports - ie, I don't think it's too much trouble for users to create the config files by hand as a one-off task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants