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 the indented syntax #2034

Merged
merged 6 commits into from
Nov 4, 2024
Merged

Add tests for the indented syntax #2034

merged 6 commits into from
Nov 4, 2024

Conversation

jamesnw
Copy link
Contributor

@jamesnw jamesnw commented Oct 24, 2024

This adds minimal coverage to the indented syntax, with focus on the areas that may change with upcoming improvements, specifically line breaks and white space parsing.

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.

Please take a look at the style guide for HRX tests, in particular the sections on irrelevant placeholders and testing one thing per spec. I think a fair number of these are more complex than they need to be in an attempt to test realistic CSS, but here terse is better than realistic.

I recommend adding some error tests as well to cover cases that are invalid, especially ones that may differ in the new system.

@jamesnw
Copy link
Contributor Author

jamesnw commented Oct 28, 2024

@nex3 Do you have a preference between adding tests to a new indented-syntax folder vs. adding .sass variations of tests to files like sass-spec/spec/directives/for.hrx?

@jamesnw jamesnw requested a review from nex3 October 30, 2024 21:05
@jamesnw
Copy link
Contributor Author

jamesnw commented Oct 30, 2024

@nex3 I've made changes per the style guide, and merged in with the existing file structure. I also found https://github.com/sass/sass-spec/tree/main/spec/non_conformant/sass and wasn't sure if it would make sense to have a syntax/sass folder to hold some of these, and maybe some of the non_conformant ones?

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.

You can generally ignore the non-conformant tests, although if you notice that you're writing tests that cover all the same cases go ahead and delete the redundant non-conformant tests.

spec/css/custom_properties/trailing_whitespace.hrx Outdated Show resolved Hide resolved
spec/arguments/invocation.hrx Outdated Show resolved Hide resolved
@jamesnw jamesnw requested a review from nex3 October 31, 2024 13:57
spec/directives/if/indented.hrx Outdated Show resolved Hide resolved
@jamesnw jamesnw requested a review from nex3 November 1, 2024 00:10

<===> sass/tab/output.css
a {
--b: c;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this behavior currently differs between the syntaxes- in a custom property, trailing whitespace is removed in Indented and condensed in Scss (see line 20).

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a design conflict here between "trailing whitespace in custom property values is observable in CSS" and "end-of-line trailing whitespaces is invisible and generally shouldn't matter", complicated further by the fact that custom property trailing whitespace nearly never matters in practice. I think given that we're planning to allow --b: c ; in the indented syntax eventually anyway, preserving the existing behavior is probably the right way forward.

@nex3 nex3 merged commit 106cd32 into sass:main Nov 4, 2024
18 checks passed
@nex3 nex3 deleted the indented-syntax branch November 4, 2024 23:35
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