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

Indented syntax improvements #2

Closed
wants to merge 40 commits into from
Closed

Conversation

jamesnw
Copy link

@jamesnw jamesnw commented Nov 27, 2024

No description provided.

@protected
void whitespace() {
void whitespace({bool consumeNewlines = false}) {
Copy link
Author

@jamesnw jamesnw Dec 4, 2024

Choose a reason for hiding this comment

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

@nex3 I'd appreciate a quick check that this approach is in the right direction. Essentially, I'm looking at every invocation of whitespace(), and determining whether a statement could end, in which case consumeNewlines should be false.

(For the WIP, I'm explicitly setting consumeNewlines: false in places I've covered, just to track progress, but those would be cleaned up.)

So far, this has covered most of my needs, with a few edge cases inside _expression.

I also want to check my assumption that the AST parsing classes should mirror these changes- for instance, should /lib/src/parse/media_query.dart mirror the changes in /lib/src/parse/stylesheet.dart:_mediaQuery?

There are also some parse*() functions in stylesheet.dart that seem to only be used by the AST, and mirror other functions- for instance parseArgumentDeclaration and _argumentDeclaration, or parseUseRule and _useRule. What are these used for, and (if the changes need to be applied there as well) how would I test them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a little unfortunate that we'll have to remember to consider each call to whitespace() in the future to determine whether or not it needs this flag, but I don't see a clearly better way to handle this. If you want to require every call site to specify this explicitly, you can write:

Suggested change
void whitespace({bool consumeNewlines = false}) {
void whitespace({required bool consumeNewlines}) {

I also want to check my assumption that the AST parsing classes should mirror these changes- for instance, should /lib/src/parse/media_query.dart mirror the changes in /lib/src/parse/stylesheet.dart:_mediaQuery?

There are also some parse*() functions in stylesheet.dart that seem to only be used by the AST, and mirror other functions- for instance parseArgumentDeclaration and _argumentDeclaration, or parseUseRule and _useRule. What are these used for, and (if the changes need to be applied there as well) how would I test them?

The other parser classes are usually used to do secondary parses of interpolated constructs. Since they're not being parsed at the stylesheet level, whitespace should never be meaningful for them, so they should always allow newlines.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks!

I was initially thinking of using whitespace(consumeNewlines: false) temporarily to track where I've checked, and then going back and removing the default value. But you're right that we want future changes to know they have to be intentional about this as well, and I'll make it required once I've made the initial pass.

@jamesnw
Copy link
Author

jamesnw commented Dec 13, 2024

@nex3 I'm close to wrapping up newline handling, but have a question.

In @supports and @import ... supports rules, I'm not sure what the correct output is. The proposed spec reads "In the indented syntax, LineBreak is not whitespace in the ImportAtRule, SupportsAtRule or MediaAtRule, except inside of parentheses or square brackets as defined by CSS syntax."

In the SCSS format, I'm seeing some curious whitespace handling that wouldn't conform to the above- here are some examples, with the failing versions commented out.

Should I match existing SCSS behavior, or does the SCSS behavior need to be fixed here?

@nex3
Copy link
Collaborator

nex3 commented Dec 14, 2024

Yes, it's an SCSS bug that no whitespace is allowed after the ( in that case.

@jamesnw
Copy link
Author

jamesnw commented Dec 17, 2024

Moved to sass#2467

@jamesnw jamesnw closed this Dec 17, 2024
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.

2 participants