Skip to content

refactor(options): use supportOptions to generate CLI options#3622

Merged
ikatyang merged 21 commits intoprettier:masterfrom
ikatyang:refactor/options
Jan 18, 2018
Merged

refactor(options): use supportOptions to generate CLI options#3622
ikatyang merged 21 commits intoprettier:masterfrom
ikatyang:refactor/options

Conversation

@ikatyang
Copy link
Copy Markdown
Member

@ikatyang ikatyang commented Jan 1, 2018 •
edited by azz
Loading

  • use supportOptions to generate CLI options
  • replace deprecated.js, jest-validate and warnings in options.js with main/options-*.js
  • this PR does not load options from external plugins, only internal plugins

Fixes #3756

@ikatyang ikatyang changed the title refactor(optios): use supportOptions to generate CLI options refactor(options): use supportOptions to generate CLI options Jan 1, 2018
@ikatyang ikatyang mentioned this pull request Jan 1, 2018
Closed
3 tasks
azz
azz reviewed Jan 1, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@azz azz left a comment

Choose a reason for hiding this comment

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

Looking good. How much more involved would it be to include external plugin options too?

Comment thread src/main/options.js
astFormat: "estree",
printer: {},
__inJsTemplate: false
printer: {}
Copy link
Copy Markdown
Member

@azz azz Jan 1, 2018

Choose a reason for hiding this comment

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

This was just to satisfy jest-validate. Might not be required now.

Copy link
Copy Markdown
Member Author

@ikatyang ikatyang Jan 1, 2018

Choose a reason for hiding this comment

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

It's also used to bypass the option validation, see L62.

@ikatyang
Copy link
Copy Markdown
Member Author

ikatyang commented Jan 1, 2018

We need to generate cli-constant dynamically, and probably need to parse cli args twice to get the proper optionInfos from plugins, it'd be a relative big change I guess, should be in another PR.

taion
taion reviewed Jan 1, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@taion taion left a comment •
edited
Loading

Choose a reason for hiding this comment

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

probably need to parse cli args twice

CLI and API args... but not a full re-parse – just need the plugins.

Comment thread src/cli/util.js

let option = constant.detailedOptionMap[key];
if (type === "api" && option === undefined) {
option = constant.apiDetailedOptionMap[key];
Copy link
Copy Markdown
Contributor

@taion taion Jan 1, 2018

Choose a reason for hiding this comment

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

This was the only consumer for apiDetailedOptionMap, so probably just remove it entirely. This only dates back to #3584. It's not part of the public API and never made it into a release anyway.

Copy link
Copy Markdown
Contributor

@taion taion Jan 1, 2018

Choose a reason for hiding this comment

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

Oops, misread. Never mind. Sorry.

@azz
Copy link
Copy Markdown
Member

azz commented Jan 9, 2018

What's the status of this PR?

@ikatyang
Copy link
Copy Markdown
Member Author

ikatyang commented Jan 9, 2018

It should be good to go if there's no further comments.

@azz
Copy link
Copy Markdown
Member

azz commented Jan 9, 2018

Looks good to me but I'm super tired and have had some 🍷 so maybe wait for someone else to review?

@ikatyang
Copy link
Copy Markdown
Member Author

ikatyang commented Jan 13, 2018

Anyone wants to review this one?

@ikatyang ikatyang mentioned this pull request Jan 17, 2018
Closed
ikatyang
ikatyang commented Jan 17, 2018
View reviewed changes
Comment thread tests_integration/cli/config/invalid/option/trailingComma
@@ -1,3 +1,3 @@
{
"trailing-comma": "wow"
"trailingComma": "wow"
Copy link
Copy Markdown
Member Author

@ikatyang ikatyang Jan 17, 2018

Choose a reason for hiding this comment

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

#3756

Copy link
Copy Markdown
Member

@azz azz Jan 17, 2018

Choose a reason for hiding this comment

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

Nice.

Copy link
Copy Markdown
Member

@azz azz Jan 17, 2018

Choose a reason for hiding this comment

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

I'll edit the OP.

@lydell
Copy link
Copy Markdown
Member

lydell commented Jan 17, 2018

Let's just merge this?

@ikatyang ikatyang merged commit dc26445 into prettier:master Jan 18, 2018
@ikatyang ikatyang deleted the refactor/options branch January 18, 2018 07:26
robert-j-webb pushed a commit to robert-j-webb/prettier that referenced this pull request Jan 18, 2018
@ikatyang @robert-j-webb
refactor(options): use supportOptions to generate CLI options (pretti…
047d898 
…er#3622)

* refactor(cli-constant): use supportOptions

* refactor(options): use supportOptions

* refactor(cli-util): use supportOptions

* fix: do not infer parser in multiparser

* chore: remove unnecessary package

* chore: trigger another travis build

* test: add kebab-case test to ensure no regression

* test: update snapshots
@lock lock Bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 18, 2019
@lock lock Bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@azz azz azz left review comments

@duailibe duailibe duailibe approved these changes

+1 more reviewer

@taion taion taion left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ikatyang @azz @lydell @duailibe @taion