Skip to content

chore: add test for number with underscore parsing#4374

Closed
Krinkle wants to merge 1 commit intoless:masterfrom
wikimedia:test-issue-2462
Closed

chore: add test for number with underscore parsing#4374
Krinkle wants to merge 1 commit intoless:masterfrom
wikimedia:test-issue-2462

Conversation

@Krinkle
Copy link
Copy Markdown
Contributor

@Krinkle Krinkle commented Oct 20, 2025

What:

Add the test case from #2462, as inpired by downstream https://gerrit.wikimedia.org/r/1197310.

Why:

Follows-up #2485.

In Less.js 2.6.0, parsing of dimensions changed so that 5_large is seen as one value, instead of as a list containing "5" and "_large".

In maintaining the wikimedia/less.php port, I recently found that we never applied this change because none of the Less.js 3.13 tests covered this behavior.

Checklist:

  • Documentation
  • Added/updated unit tests
  • Code complete

In Less.js 2.6.0, parsing of dimensions changed so that `5_large`
is seen as one value, instead of as a list containing "5" and "_large".

In updating the Less.php port, we forgot to consider this change
because none of the Less.js 3.13 tests seem to cover this behavior.

Follows-up less#2485.

This adds the test case from less#2462,
as inpired by downstream https://gerrit.wikimedia.org/r/1197310.
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Oct 20, 2025
@puckowski
Copy link
Copy Markdown
Contributor

@Krinkle Thank you for the PR.

I think adding a test for #2485 is a good idea.

I'll give some time for other maintainers to weigh in.

@puckowski puckowski self-requested a review October 25, 2025 13:22
Copy link
Copy Markdown
Contributor

@puckowski puckowski left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, though I think we may not land this quickly as #4378 is now pending and is a significant rewrite of the tests.

}
})

// https://github.com/less/less.js/issues/2462
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is good information, though I prefer to drop the comment with issue number as the comment itself is not used as part of the test.

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Mar 8, 2026
@matthew-dean
Copy link
Copy Markdown
Member

Superseded by #4406 (rebased onto current master). Thank you @Krinkle!

@Krinkle Krinkle deleted the test-issue-2462 branch March 10, 2026 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants