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

Set trailingComma default value to es5(outdated) #6910

Closed
wants to merge 0 commits into from
Closed

Set trailingComma default value to es5(outdated) #6910

wants to merge 0 commits into from

Conversation

fisker
Copy link
Member

@fisker fisker commented Nov 10, 2019

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

✨Try the playground for this PR✨

fisker
fisker commented Nov 10, 2019
View reviewed changes
tests/angular_component_examples/jsfmt.spec.js Outdated
@@ -1,2 +1,2 @@
run_spec(__dirname, ["typescript"], { trailingComma: "none" });
run_spec(__dirname, ["typescript"]);
Copy link
Member Author

@fisker fisker Nov 10, 2019 •
edited

Choose a reason for hiding this comment

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

I change the test orders to help review, snap changes should only be options part

fisker
fisker commented Nov 10, 2019
View reviewed changes
tests/css_scss/__snapshots__/jsfmt.spec.js.snap Outdated
@@ -1987,7 +1987,7 @@ $my-map: (
"foo": 1,
// Comment
"bar": 2,
// Comment
// Comment,
Copy link
Member Author

Choose a reason for hiding this comment

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

this should be a scss bug, need fix with another PR

Copy link
Member Author

Choose a reason for hiding this comment

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

issue: #6911

@fisker fisker mentioned this pull request Nov 10, 2019
Merged
4 tasks
fisker
fisker commented Nov 10, 2019
View reviewed changes
tests/angular_interpolation/__snapshots__/jsfmt.spec.js.snap Outdated
@@ -15,7 +15,7 @@ printWidth: 80
advancedSearchService.patientInformationFieldsRow2 &&
advancedSearchService.patientInformationFieldsRow2.indexOf(
advancedSearchService.formElementData.customFieldList[i].customFieldType
) !== -1
) !== -1,
Copy link
Member Author

@fisker fisker Nov 10, 2019 •
edited

Choose a reason for hiding this comment

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

PR #6912

lydell
lydell reviewed Nov 10, 2019
View reviewed changes
src/language-js/options.js Outdated
{
value: "es5",
description:
"Trailing commas where valid in ES5 (objects, arrays, etc.)"
},
{ value: "none", description: "No trailing commas." },
Copy link
Member

Choose a reason for hiding this comment

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

Why is this moved?

Copy link
Member Author

Choose a reason for hiding this comment

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

playground shows none above es5

Copy link
Contributor

Choose a reason for hiding this comment

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

I’d prefer if the order wasn’t changed, as that way, es5 is still implied to be between none and all.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ExE-Boss why? after changing default to es5, shouldn't we put it on the top?

@fisker fisker changed the base branch from master to next November 10, 2019 19:33
@fisker
Copy link
Member Author

fisker commented Nov 11, 2019 •
edited

waiting for two blockers #6918(update: merged) and #6912

alexander-akait
alexander-akait requested changes Nov 11, 2019
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Someone can give a link where we decided that we will change it in the next version, because we only talk about what we may change, but I did not see where we decided to change it

@thorn0
Copy link
Member

thorn0 commented Nov 11, 2019

#6888 (comment)

@lydell
Copy link
Member

lydell commented Nov 11, 2019

Changing the default of trailingComma was proposed in the original 2.0 issue #3503 (after having being requested in other issues) and I’ve seen 0 mentions against it.

@fisker fisker closed this Nov 15, 2019
@fisker
Copy link
Member Author

fisker commented Nov 15, 2019

accidentlly force-pushed a wrong commit, auto closed, please reopen

@lydell
Copy link
Member

lydell commented Nov 15, 2019

The "Reopen" button is disabled for me. Its tooltip says: "There are no new commits on the fisker:trailing-comma-es5 branch."

@fisker fisker mentioned this pull request Nov 15, 2019
Merged
4 tasks
@fisker
Copy link
Member Author

fisker commented Nov 15, 2019

new PR #6963

@fisker fisker changed the title Set trailingComma default value to es5 Set trailingComma default value to es5(outdated) Nov 15, 2019
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Feb 13, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lydell lydell lydell left review comments

@ExE-Boss ExE-Boss ExE-Boss left review comments

@alexander-akait alexander-akait alexander-akait requested 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
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@fisker @thorn0 @lydell @ExE-Boss @alexander-akait