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

Add tests for comments in more expression-like contexts #2000

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Jun 17, 2024

See sass/dart-sass#2263

[skip dart-sass]

@ntkme
Copy link
Contributor

ntkme commented Jun 18, 2024

I wonder if we should omit the whitespace in between silent comment and sibling tokens during output if possible?

@nex3
Copy link
Contributor Author

nex3 commented Jun 19, 2024

It's not generally safe to omit that whitespace entirely—for example, foo//\nbar is very different than foobar. There are some cases where we could fold more aggressively than we do now, but that's generally true with newlines adjacent to trailing spaces, not just silent comments.

================================================================================
<===> comment/progid/before_close_paren/silent/input.scss
a {
b: progid:c(d //
Copy link
Member

Choose a reason for hiding this comment

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

If there are multiple lines with //, are they considered a single silent comment, or multiple silent comments?

For example, would this block:

a {
  b: progid:c(d // foo
                // bar
    );
}

result in this?:

a {
  b: progid:c(d

    );
}

And if so, is that OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are not considered a single comment, but the whitespace around them is still considered "adjacent" so you won't end up with multiple newlines.

@ntkme
Copy link
Contributor

ntkme commented Jun 20, 2024

There are some cases where we could fold more aggressively than we do now

If we treat silent comment as a kind of whitespace when processing AST, I think this can potentially allow us to fold it more aggressively based on context, even under the current logic.

For example, if silent comment is treated as whitespace:
a { b: calc(c<whitespace><silent-comment><newline><whitespace>) } can be parsed as a { b: calc(c<whitespace>) }, and this whitespace can be completely striped based on its context.

Not sure if there is any corner case, and I haven't looked at the implementation details so not sure how much work it is. If it's too much work we can just leave it as is.

@nex3 nex3 merged commit e9f7340 into main Jun 20, 2024
12 checks passed
@nex3 nex3 deleted the silent-comment-everywhere branch June 20, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants