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

Allow trailing comma in scss list and maps #4317

Merged
merged 11 commits into from Apr 18, 2018

Conversation

Projects
None yet
6 participants
@ad1992
Copy link
Contributor

commented Apr 15, 2018

fix #4076

ad1992 added some commits Apr 15, 2018

@@ -679,6 +679,7 @@ function genericPrint(path, options, print) {
)
])
),
shouldPrintTrailingComma(options.trailingComma, node) ? "," : "",

This comment has been minimized.

Copy link
@j-f1

j-f1 Apr 15, 2018

Member

How do you feel about refactoring this to options.trailingComma && node.groups.length > 1 instead of a separate function?

Also, by testing the truthiness of options.trailingComma, instead of checking for "all", we can print trailing commas if the value is "es5".

@@ -679,6 +679,7 @@ function genericPrint(path, options, print) {
)
])
),
node.groups.length > 1 && options.trailingComma === "es5" ? "," : "",

This comment has been minimized.

Copy link
@j-f1

j-f1 Apr 15, 2018

Member

Sorry, I was unclear in my first review. We should print a comma here if options.trailingComma !== "none", since we don’t care which option the user picks in this case (all or es5).

@j-f1

j-f1 approved these changes Apr 15, 2018

Copy link
Member

left a comment

Looks good to me, but I’d like to see what a few other maintainers think.

@j-f1 j-f1 requested review from lydell and evilebottnawi Apr 15, 2018

@lydell

lydell approved these changes Apr 15, 2018

Copy link
Collaborator

left a comment

Looks good to me! You’ll have to be prepared to make a bugfix PRs after this has been released though, because you’ll never know what sideeffects changes to the CSS printing might have, unfortunately.

}
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
a {
margin: $bar;

This comment has been minimized.

Copy link
@lydell

lydell Apr 15, 2018

Collaborator

What is this test trying to say? Are you specifically trying to remove that trailing comma, or is this just an observation you made? I guess it doesn’t really matter what happens to the comma in this case. Just curious.

This comment has been minimized.

Copy link
@ad1992

ad1992 Apr 15, 2018

Author Contributor

@lydell here the trailing comma should not be shown as it was previously in line 9. I had just added trailingComma: "es5" in jsfmt.spec.js so this is showing here as all the tests are again run for trailingComma: "es5"

@ad1992

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2018

Sure I understand and will take care of the bug fixes when the release goes live.

$colors: (
"red",
"blue"
);

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 15, 2018

Member

Let's add tests for nested maps and lists inside maps

This comment has been minimized.

Copy link
@ad1992

ad1992 Apr 16, 2018

Author Contributor

I have added the tests. is this the expected behaviour when nested playgroundlink ?

According to style guide trailing comma should not be showing when in single line so is there any better way of detecting if its exceeding printWidth except for looping through the node node and checking the length occupied and then decide whether to put trailing comma ?

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 16, 2018

Member

@ad1992 looks like we have problem with nested maps 😕 but it is not part of your PR, will be great if you create issue around this for future fix 👍

@@ -679,6 +679,7 @@ function genericPrint(path, options, print) {
)
])
),
node.groups.length > 1 && options.trailingComma !== "none" ? "," : "",

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 15, 2018

Member

Can we add check what it is isSCSS(), in theory we can break less code, some projects can have scss/less files togethe (legacy or migration period)

This comment has been minimized.

Copy link
@ad1992

ad1992 Apr 15, 2018

Author Contributor

@evilebottnawi I didn't get you here. What check you want me to add here exactly ?

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 16, 2018

Member

@ad1992 nested maps, when variable contain map and each key contain other map/list

This comment has been minimized.

Copy link
@ad1992

ad1992 Apr 16, 2018

Author Contributor

sorry i guess there is some confusion. i understood about adding nested map and lists. But i didnt get Can we add check what it is isSCSS(), in theory we can break less code, some projects can have scss/less files togethe (legacy or migration period

So is there any specifice check you meant in the above comment except for nested map and lists ?

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Apr 16, 2018

Member

@ad1992 parsers some times change ast and introduce new nodes, better check directly what is SCSS to avoid break something in future

This comment has been minimized.

Copy link
@ad1992

ad1992 Apr 16, 2018

Author Contributor

@evilebottnawi so you mean we need to make sure trailing comma is added only in case of scss.
I referred to parser-postcss.js file and added check for isSCSS.
Is this fine ?
Sorry since I am new to prettier codebase and AST, I am asking too many questions.

ad1992 added some commits Apr 16, 2018

@evilebottnawi

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

/cc @lydell @j-f1 second round review

@j-f1

This comment has been minimized.

Copy link
Member

commented on src/language-css/printer-postcss.js in 0734174 Apr 16, 2018

Can you extract this to a utils.js file in this directory so the same code can be used here and in the parser?

ad1992 added some commits Apr 16, 2018

@ad1992

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2018

@j-f1 made the changes. Can we make yarn lint as a part of pre-commit hook ? so that user is forced to fix the linting issues before he pushes the commit.

@j-f1

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

@ad1992 I’m personally not a fan of pre-commit hooks, since they make it take longer to commit. If you can, I’d recommend installing a Prettier editor integration so the code gets formatted when you save.

@lipis

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

How much longer?! It's not like there is a need to commit every 8 minutes.. One can use both.. and sometimes the settings in the editor are different than and it's not guaranteed that the plugin will manage to read all the strange settings (like ignoring bunch of directories, or running prettier only on specific dirs).

@ad1992

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2018

@lipis making it part of pre-commit will take the same time yarn lint takes. So if we have pre-commit hook then it will be helpful for all the contributors as they will be forced to fix the issues and they don't have to set up in local.
Actually if we add a pre-commit then we should add two things

yarn lint 
yarn test

So that all checks pass in travis.

@j-f1

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

@ad1992 Since the Travis run for the last commit took ten minutes, it’s reasonable to expect that each commit would take at least five minutes to be saved if yarn test was included. IMO this is an excessively long wait, especially since you can always push a commit that fixes the tests, or your PR is supposed to add a failing test case.

@ad1992

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2018

@j-f1 I understand and agree that it will make the commit process slower. But in case of pre-commit hook one can always commit with -nm if the user wants to skip it. While pushing the commits I often forget to make the linting fixes and when travis fails I realize I forgot to make changes and push the linting fix and wait for it to pass. So I suggested if pre-commit hook if it can make it better. No issue if we don't add it, I understand your concern, and will make the local settings changes as you suggested 👍

@lydell

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2018

I usually use a pre-push hook (in addition to an editor integration). I commit way more often than I push. Makes the push slower, but faster than having CI fail.

@evilebottnawi

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

/cc @lydell @j-f1 second round review 😄

@lydell

lydell approved these changes Apr 18, 2018

@ad1992

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2018

@j-f1 anything else I can do to help get this merged?

@duailibe duailibe merged commit 7f20ffc into prettier:master Apr 18, 2018

4 checks passed

codecov/patch 100% of diff hit (target 80%)
Details
codecov/project 95.83% (+<.01%) compared to 0a22f5e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@duailibe

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

@ad1992 Thanks for the contributions! 💯

@ad1992 ad1992 deleted the ad1992:aakansha-prettier branch Apr 18, 2018

@lipis lipis added this to the 1.13 milestone May 9, 2018

@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.
You can’t perform that action at this time.