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.

Sign up for GitHub

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

Jump to bottom

chore(playground): add new options #3350

Merged
merged 1 commit into from
Dec 5, 2017
Merged

chore(playground): add new options #3350

merged 1 commit into from
Dec 5, 2017

Conversation

ikatyang
Copy link
Member

@ikatyang ikatyang commented Nov 29, 2017 •
edited by azz

suchipi
suchipi approved these changes Nov 29, 2017
View reviewed changes
Copy link
Member

@suchipi suchipi left a comment

Choose a reason for hiding this comment

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

It'd be cool if someday the HTML was generated from cli-constant

src/cli-constant.js
@@ -82,7 +82,6 @@ const detailedOptions = normalizeDetailedOptions({
category: CATEGORY_FORMAT,
forwardToApi: true,
description: "Include parentheses around a sole arrow function parameter.",
default: "avoid",
Copy link
Member

Choose a reason for hiding this comment

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

Are you going to fill this back in once we've decided on the new default?

Copy link
Member Author

@ikatyang ikatyang Nov 29, 2017 •
edited

Choose a reason for hiding this comment

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

Oops, forgot to mention why I removed this line: we use the default value from options.js for all format options.

prettier/src/cli-util.js

Lines 574 to 576 in fbbfa52

if (optionCamelName in apiDefaultOptions) {
return apiDefaultOptions[optionCamelName];
}

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that's feels a bit unintuitive, but changing it is out of the scope of this PR

@azz
Copy link
Member

azz commented Dec 1, 2017

Currently the playground uses the released version of Prettier (from package.json) so this won't actually work if we merge it.

@duailibe duailibe added this to the 1.9 milestone Dec 1, 2017
@duailibe
Copy link
Member

duailibe commented Dec 1, 2017 •
edited

I added the 1.9 milestone just to help us remember it's related (and merge after release) but feel free to remove if you think someone might think this is a blocker.

azz
azz requested changes Dec 1, 2017
View reviewed changes
Copy link
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.

(so we don't accidentally merge)

@suchipi
Copy link
Member

suchipi commented Dec 2, 2017

So to clarify, this should be merged post release?

@azz
Copy link
Member

azz commented Dec 2, 2017

Yes. After prettier's own dependency on prettier is updated to 1.9.0

@azz azz merged commit f3e7ca2 into prettier:master Dec 5, 2017
@ikatyang ikatyang deleted the chore/playground-new-options branch December 5, 2017 10:28
@ikatyang ikatyang mentioned this pull request Dec 7, 2017
Merged
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@suchipi suchipi suchipi approved these changes

@azz azz azz approved these changes

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
1.9
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ikatyang @azz @duailibe @suchipi