Fixed wrong trailing comma position after comment in scss#16617
Fixed wrong trailing comma position after comment in scss#16617fisker merged 18 commits intoprettier:mainfrom
Conversation
|
Thanks for the fast response, appreciate it. |
sosukesuzuki
left a comment
sosukesuzuki
left a comment
There was a problem hiding this comment.
Oh full tests fail on this change. Please fix it.
You can run full tests by FULL_TEST=1 yarn test
Ma-hawaj
force-pushed
the
trailing-comma-fix
branch
from
94f7e52 to
db40272
Compare
|
@sosukesuzuki Can you assist me with this ? This is the output Doc {
"type": "group",
"contents": {
"type": "indent",
"contents": {
"type": "fill",
"parts": [
[
[
[
[
[
"",
"overlay"
],
{
"type": "group",
"contents": [
":",
{
"type": "line"
}
],
"break": false
}
],
[
"1202",
""
]
],
{
"type": "line-suffix",
"contents": [
" ",
"// The comma shoud be printed before the comment when trailing-comma = es5"
]
}
],
{
"type": "break-parent"
}
]
]
}
},
"break": false
}
|
|
@Ma-hawaj You can know about the full tests on https://github.com/prettier/prettier/blob/main/CONTRIBUTING.md#deeper-testing |
|
Seems like there is a bug with the parser it self since if there is a trailing comma it doesn't consider the comment as part of the comma separated group while when there is not it counts it as a part of the comma separated group. |
|
Can be related to #16583 |
|
It is now working functionally but the parsing tree a bit different when reparsing so the full test fails. |
Co-authored-by: fisker Cheung <[email protected]>
Co-authored-by: fisker Cheung <[email protected]>
Co-authored-by: Cyrille David <[email protected]>
|
Currently it has this weird behavior in the main release code. .simplification {
foo: (
calc(), // It is a comment
);
}
.simplification {
foo: (
calc() // not a comment anymore
);
}
$z-indexes: (
header: 1035,
header: 1035,
overlay: 1202, // TODO
header: 1035,
header: 1035,
);Output: .simplification {
foo: (calc(), // It is a comment);
}
.simplification {
foo: (
calc() // not a comment anymore
);
}
$z-indexes: (
header: 1035,
header: 1035,
overlay: 1202,
// TODO
header: 1035,
header: 1035,
);
which also breaks the code for the first example. |
|
I'm working on it. |
fisker
force-pushed
the
trailing-comma-fix
branch
from
955a537 to
244cf81
Compare
| $my-map: ( | ||
| "foo": 1, | ||
| // Comment | ||
| "bar": 2, // Comment | ||
| ); |
There was a problem hiding this comment.
This is actually wrong, the first comment should not print on separate line, but it's a different issue, we can deal with it later.
There was a problem hiding this comment.
OK, I thought it is the default behavior since it was like this in the released version.
There was a problem hiding this comment.
It's bug, but we don't have a comment attach logic in CSS printer, it's not easy to fix(I guess).
There was a problem hiding this comment.
I could be wrong, if we always print comments with other elements instead of a separate element, it should be fixed? Don't know.
|
@Ma-hawaj This should be good enough to merge, but some other cases need to be improved. Eg #16617 (comment), #16743 , and the TODO comment in code. |
|
@fisker Thanks for your support. |
4 participants
Description
Fixes #16599
Fixes #6920
Checklist
docs/directory).changelog_unreleased/*/XXXX.mdfile followingchangelog_unreleased/TEMPLATE.md.guidelines.
✨Try the playground for this PR✨