Skip to content

Fix brace expansion with range going down #17591

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No 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? No Sign in to your account

Merged
merged 6 commits into from
Apr 7, 2025
Merged

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Apr 7, 2025

This PR fixes an issue where a test was setup wrong. While fixing the test, I also noticed that the actual computation was wrong. There are a few cases we have to make sure they work when working with ranges:

  • 0..10..5 — range going from low to high, with a positive step. This should result in 0 5 10. Already worked.
  • 0..10..-5 — range going from low to high, with a negative step. This should result in 10 5 0. This one was broken.
  • 10..0..5 — range going from high to low, with a positive step. This should result in 10 5 0. This one was broken.
  • 10..0..-5 — range going from high to low, with a negative step. This should result in 0 5 10. Already worked.

This technically means that there are multiple ways to do the same thing. And in case of Tailwind CSS the order doesn't really matter because we apply custom sort rules for utilities. Luckily, the order here doesn't have any influence on the actual generated CSS so it's not the end of the world to support it.

Intellisense does use a more heavy brace expansion library, and if we don't support this, it also means that you can see a preview of generated classes without generating the actual classes. This also fixes that.

E.g.: Note how the preview shows the correct values, but nothing has been generated (this is before the change of this PR).

image

Fixes: #17576

Test plan

  1. Hoisted the test (a/{10..0..5}/b), which failed
  2. Fixed the implementation to make the test pass
  3. Added an additional test (a/{10..0..-5}/b)

@RobinMalfait RobinMalfait requested a review from a team as a code owner April 7, 2025 11:46
This now completes all versions:
- a/{0..10..5}/b    — increasing, positive step
- a/{0..10..-5}/b   — increasing, negative step
- a/{10..0..5}/b    — decreasing, positive step
- a/{10..0..-5}/b   — decreasing, negative step
Copy link
Contributor

@thecrypticace thecrypticace left a comment

Choose a reason for hiding this comment

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

I really should unify the brace expansion code in IntelliSense with the one we've got here.

Looks good 👍

@RobinMalfait RobinMalfait merged commit f66d287 into main Apr 7, 2025
7 checks passed
@RobinMalfait RobinMalfait deleted the fix/issue-17576 branch April 7, 2025 13:48
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unused test data in brace-expansion.test.ts
2 participants