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] Dart implementation #2467

Open
wants to merge 48 commits into
base: main
Choose a base branch
from

Conversation

jamesnw
Copy link
Contributor

@jamesnw jamesnw commented Dec 16, 2024

Blocked until proposal is accepted.

[skip sass-embedded]

@jamesnw
Copy link
Contributor Author

jamesnw commented Dec 16, 2024

@nex3 This is ready for review. Here are some questions I wanted to flag as you review. Thanks!

  1. allowNewlines is set to true in some cases, like in parser* functions or in scss.dart, where the value is non-impactful. Should this usage be differentiated somehow for clarity?
  2. There are some places, like in _atRootQuery, where allowNewlines is true because it is inside parentheses. I opted to not use the _inParentheses variable except for one place where the consumption did rely on it, in _supportsCondition. Would it be more idiomatic to use _inParentheses, even if the consumption is in the same function?
  3. In the + include syntax, I am not supporting whitespace after the + as it can be part of a selector. This differs from the = mixin syntax- should they be the same?
  4. I added paren/bracket context to almostAnyValue, with the only change being that they must be matched. That means the error for a:has(d[)] will be changed from "Expected identifer" to "Expected ]". Since almostAnyValue is used for more than selectors, should I make this functionality opt-in, dependent on a parameter? Based on the places where this is used, I don't foresee issues with bracket matching, but want to verify.

Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

@nex3 This is ready for review. Here are some questions I wanted to flag as you review. Thanks!

  1. allowNewlines is set to true in some cases, like in parser* functions or in scss.dart, where the value is non-impactful. Should this usage be differentiated somehow for clarity?

For classes where it's universally non-impactful, I'd define a local _whitespace() function that takes no arguments with a comment explaining that the value isn't relevant.

  1. There are some places, like in _atRootQuery, where allowNewlines is true because it is inside parentheses. I opted to not use the _inParentheses variable except for one place where the consumption did rely on it, in _supportsCondition. Would it be more idiomatic to use _inParentheses, even if the consumption is in the same function?

I think how it is now is better.

  1. In the + include syntax, I am not supporting whitespace after the + as it can be part of a selector. This differs from the = mixin syntax- should they be the same?

There's already an odd inconsistency here where whitespace is allowed after = but not after +, so I think it's fine to expand that to allowing newlines as well. It's possible that we should deprecate that, but it's not in-scope for this change.

  1. I added paren/bracket context to almostAnyValue, with the only change being that they must be matched. That means the error for a:has(d[)] will be changed from "Expected identifer" to "Expected ]". Since almostAnyValue is used for more than selectors, should I make this functionality opt-in, dependent on a parameter? Based on the places where this is used, I don't foresee issues with bracket matching, but want to verify.

It's fine to change this error. Arguably the bracket error is clearer anyway.

lib/src/parse/keyframe_selector.dart Outdated Show resolved Hide resolved
lib/src/parse/parser.dart Outdated Show resolved Hide resolved
lib/src/parse/parser.dart Outdated Show resolved Hide resolved
lib/src/parse/sass.dart Outdated Show resolved Hide resolved
var next = scanner.peekChar();
return next.isNewline ||
next == null ||
(next == $semicolon && scanner.peekChar(1).isNewline);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should treat ; even without a newline as the end of a statement, and just let the parser throw an error when it encounters more text afterwards.

allowNewlines: allowNewlines ||
bracketList ||
_inParentheses ||
wasInExpression);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this trigger on wasInExpression?

brackets.add(opposite(bracket));

case $rparen || $rbracket:
if (brackets.isEmpty) break loop;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we're now explicitly parsing matching brackets here, it's probably best to throw a meaningful error here.

Note that this is technically a breaking change whether or not we throw an error here: [foo#{"]:is(bar"}) {x: y} is currently valid, but will become an error here. I think it's vanishingly unlikely that anyone is doing that, though, so it should be fine to break, but we should add a "potentially breaking bug fix" note in the changelog about it.

lib/src/parse/stylesheet.dart Outdated Show resolved Hide resolved
lib/src/parse/stylesheet.dart Outdated Show resolved Hide resolved
lib/src/parse/stylesheet.dart Outdated Show resolved Hide resolved
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