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

New cli #167

Merged
merged 9 commits into from
Aug 16, 2021
Merged

New cli #167

merged 9 commits into from
Aug 16, 2021

Conversation

hildjj
Copy link
Contributor

@hildjj hildjj commented Jun 14, 2021

Split into several chunks:

  • Compile the existing CLI with rollup
  • Add tests to the exisitng CLI
  • Move the CLI to commander, updating tests
  • Add -t/-T
  • Fix Add -c / --config CLI option #161 by adding support for .js/.cjs config files and being careful about the command line overriding the config file, but the config file overriding defaults.

@hildjj
Copy link
Contributor Author

hildjj commented Jun 15, 2021

This needs some testing on windows before it's ready.

Copy link
Member

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

In general very good, but test file should be read after the parser generation and changing output type from source to parser can lead to some unintended results. I think, that this should be fixed.

The other stuff are minor but it will be great if you look at them.

CHANGELOG.md Outdated Show resolved Hide resolved
bin/peggy.mjs Show resolved Hide resolved
bin/peggy.mjs Show resolved Hide resolved
bin/peggy.mjs Outdated Show resolved Hide resolved
bin/peggy.mjs Outdated Show resolved Hide resolved
bin/peggy.mjs Outdated Show resolved Hide resolved
bin/peggy.mjs Outdated Show resolved Hide resolved
rollup.config.js Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
bin/peggy.mjs Show resolved Hide resolved
Copy link
Member

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

This two changes fixes the Windows build

test/cli/run.spec.ts Outdated Show resolved Hide resolved
test/cli/run.spec.ts Outdated Show resolved Hide resolved
@Mingun Mingun mentioned this pull request Jul 8, 2021
rollup.config.js Show resolved Hide resolved
rollup.config.js Show resolved Hide resolved
@hildjj
Copy link
Contributor Author

hildjj commented Aug 12, 2021

Sorry it's taken so long, but I'm finally addressing all of these comments.

@Mingun, you said "changing output type from source to parser can lead to some unintended results". Can you say more about that? My thought was that if you're supplying a test, we should get a parser back instead of source, so that we can call that generated code.

@hildjj
Copy link
Contributor Author

hildjj commented Aug 14, 2021

I would expect that --test is fully additive, so parser is still generated and written to the --output

I can see this point of view. There have been a few times where I've wanted to save the output of the test, but I suppose I can always just capture stdout or add another option later if it becomes important.

@hildjj
Copy link
Contributor Author

hildjj commented Aug 14, 2021

After thinking about it some more, I don't want to add variadic options because it would break backward-compatibility for some uses of the command line.

@Mingun
Copy link
Member

Mingun commented Aug 14, 2021

After thinking about it some more, I don't want to add variadic options because it would break backward-compatibility for some uses of the command line.

Yes, I also thinking about this after you write this . So may be later.

I think it is almost done, but I don't like the way how the options combined from the config file, command line JSON and command line parameters -- it is too sophisticated and non-obvious. I suggest to change handling, as I did in hildjj/peggy@new-cli...Mingun:new-cli (and also fix couple of other issues).

Also, If I understood correctly, do you agree with me that it is worth reworking the way the --test is handled?

@hildjj
Copy link
Contributor Author

hildjj commented Aug 14, 2021

I have reworked how --test behaves, as discussed above. I'm just fixing up the tests now before pushing the update.

I agree that the config file stuff is a little subtle, but I think it was always that way. Can you explain a little more about how you changed it? I'm not seeing how yours is different from mine in the link above.

@Mingun
Copy link
Member

Mingun commented Aug 14, 2021

Conceptually I did the following:

  1. I introduce DEFAULTS const with all options that have a default value
  2. Then I save parsed config from the commander to the cliOptions.
  3. Then remove from cliOptions all keys that have default values (as defined in the DEFAULTS)
  4. Then build new options object as
Object.assign({}, DEFAULTS, extraOptionsFile, extraOptions, cliOptions)

To clarify: I just made forming options a little more straightforward (at least it looks so for me)

@hildjj
Copy link
Contributor Author

hildjj commented Aug 14, 2021

That makes sense. I'll work in something like that.

@hildjj
Copy link
Contributor Author

hildjj commented Aug 14, 2021

As a compromise on variadic options, and to make everything consistent, you can now specify a comma-separated list for --dependency and --plugin

@Mingun
Copy link
Member

Mingun commented Aug 14, 2021

As for dancing around config, I've just create an issue in the commander, so, if it will be implemented, we could switch to the built-in solution: tj/commander.js#1584

… edge cases in the cli. Ensure that cli options are documented consistently. Make 'bare' a valid cli output type. Make multi-options all take comma-separated lists. Parser got generated with fix from peggyjs#170.  Ensure map files get cleaned up.
@hildjj
Copy link
Contributor Author

hildjj commented Aug 15, 2021

I think I have all of the requested changes in now. The defaults handling is better than what I had before, but wasn't able to be as clean as you had, because I wanted arrays and objects in the options to be additive instead of replacing what the earlier option was.

…st, which is waiting on an update to ts-jest. Fix tests to await results.
@Mingun
Copy link
Member

Mingun commented Aug 16, 2021

Ok, it seems good. LGTM

@hildjj hildjj merged commit 11f46d9 into peggyjs:main Aug 16, 2021
@hildjj hildjj deleted the new-cli branch August 16, 2021 17:51
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.

Add -c / --config CLI option
3 participants