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

feat(support-info): add options field #3433

Merged
merged 17 commits into from Dec 31, 2017

Conversation

ikatyang
Copy link
Member

@ikatyang ikatyang commented Dec 7, 2017

This will unblock #3352. cc @duailibe

And I'm going to refactor options.js/cli-constant.js using getSupportInfo() in another PR to address #3350 (comment), #3340 (comment), and #3352 (comment).

@duailibe
Copy link
Member

duailibe commented Dec 7, 2017

Thank you for doing this!

Looks good to me, but I'd wait for another review :)

@@ -386,17 +387,12 @@ const minimistOptions = {
)
};

const usageSummary = `
Usage: prettier [options] [file/glob ...]
const usageSummary = dedent(`
Copy link
Member

@lydell lydell Dec 7, 2017

Choose a reason for hiding this comment

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

Isn’t dedent supposed to be used as a template tag?

@duailibe duailibe added this to the 1.9.2 milestone Dec 7, 2017
@duailibe
Copy link
Member

duailibe commented Dec 7, 2017

If we can get it in 1.9.2, we wouldn't have to wait another release to do #3352 (since the site's playground only uses the released version)

src/support.js Outdated
description: "Which parser to use.",
choices: [
{ value: "flow", description: "Flow" },
{ value: "babylon", description: "JavaScript" },
Copy link
Member

Choose a reason for hiding this comment

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

Can you reorder flow and babylon? Just to keep the same order from the docs and the playground?

(not that it makes that much difference but still...)

@duailibe
Copy link
Member

duailibe commented Dec 8, 2017

I've started working on the playground rewrite and it's looking pretty good, I have it almost done with preact, with no build steps :-)

One of the problems I faced now is when in a PR preview, which version should I pass to getSupportInfo to get the unreleased opts? It would be nice if it was always getSupportInfo() so I don't have to special case PR previews or the official playground, but we don't bump the package.json until we release a new version...

(Actually the PR preview changes the package.json to pr+XXXX but that can be changed.. I don't know what we should change it to)

I can't simply append -beta in PR previews because new stuff is likely to be on 1.10 and then new options would not be enabled (1.9.1-beta < 1.10). So another option would be to always bump the minor when inside a PR preview, but that might be confusing to users because we may do a patch release before the next minor? I'm not sure what I should do...

Let me know if I wasn't clear!

@ikatyang
Copy link
Member Author

ikatyang commented Dec 8, 2017

getSupportInfo(null, { showUnreleased: !!isPR })?

@duailibe
Copy link
Member

duailibe commented Dec 9, 2017

@ikatyang that would work. I hope that’s not too hackish though

@duailibe duailibe removed this from the 1.9.2 milestone Dec 11, 2017
@duailibe duailibe mentioned this pull request Dec 11, 2017
5 tasks
@ikatyang
Copy link
Member Author

Anyone wants to do another review? I'd like to merge it and send a followed up PR.

@duailibe
Copy link
Member

@azz @lydell @suchipi anyone have any comments about this? Can we merge it?

const previousVersion = testVersions[index - 1];
const previousInfo = getCoreInfo(previousVersion);
test(`with version ${previousVersion} -> ${version}`, () => {
expect(snapshotDiff(previousInfo, info)).toMatchSnapshot();
Copy link
Member

Choose a reason for hiding this comment

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

Neat idea with snapshotDiff!

@azz
Copy link
Member

azz commented Dec 16, 2017

Unless I'm reading this wrong we now have two copies of each option definition (cli-constant.js and support.js)?

@ikatyang
Copy link
Member Author

I have a followed up PR for it, see ikatyang/prettier@feat/support-options...ikatyang:refactor/options, or should I add them in this PR?

@duailibe
Copy link
Member

duailibe commented Dec 29, 2017

I fixed all the conflicts, @azz can you take a look?

I had to change the getSupportInfo.. I'm not sure how useful it is to load external plugins there since the information is about prettier's own functionality?

This unblocks #3573

@duailibe duailibe mentioned this pull request Dec 29, 2017
@azz
Copy link
Member

azz commented Dec 30, 2017

We need to split it into multiple files:

  • src/common/options.js

    • --parser
    • --print-width
    • --tab-width
    • --use-tabs
    • --insert-pragma
    • --require-pragma
  • laguage-js/options.js:

    • --no-semi
    • --single-quote
    • --no-bracket-spacing
    • --jsx-bracket-same-line
    • --arrow-parens
    • --trailing-comma
  • language-markdown/options.js

    • --prose-wrap

And then expose them as options on the printer objects.

@j-f1
Copy link
Member

j-f1 commented Dec 30, 2017

--single-quote is also used by Markdown and CSS.
--no-bracket-spacing is also used by GraphQL.

I’m wondering if it would be a good idea to enable --prose-wrap on JSDoc comments.

@azz
Copy link
Member

azz commented Dec 30, 2017

That's fine, we should re-declare those flags in the printers that use them.

@@ -132,6 +133,7 @@ const printers = {
};

module.exports = {
options,
Copy link
Member

Choose a reason for hiding this comment

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

options should probably be a property of a specific printer rather than a plugin, as a plugin can provide multiple languages/printers. In this case it should be printers.estree.options

@duailibe
Copy link
Member

@ikatyang is there anything else to tackle on this PR?

@ikatyang
Copy link
Member Author

@duailibe No, the CLI option generation will be in another PR. This one should be good to go.

@azz azz merged commit 6c0dd74 into prettier:master Dec 31, 2017
@ikatyang ikatyang deleted the feat/support-options branch December 31, 2017 04:38
This was referenced Dec 31, 2017
@azz azz added this to the 1.10 milestone Jan 7, 2018
@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.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants