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

Update UnitUtil tests and code #1211

Merged
merged 1 commit into from
Feb 27, 2025
Merged

Update UnitUtil tests and code #1211

merged 1 commit into from
Feb 27, 2025

Conversation

dpvc
Copy link
Member

@dpvc dpvc commented Feb 5, 2025

This PR updates the tests for UnitUtil.ts and adjusts the code:

  • In original line 98, since we know (from line 94) that we have mu units, that means value will not be an empty string, and so the || '1' is not needed.
  • In original line 146, we know factor is defined, since the UNIT_CASES.get() function in the previous line will return the value for 'pt' (line 65) if the unit itself is not found. So unless someone deletes the 'pt' value, the factor should always be defined, and if it turns out to be 0, then factor * m will be 0, so there is no need for the test.

@dpvc dpvc requested a review from zorkow February 5, 2025 18:34
@dpvc dpvc added this to the v4.0 milestone Feb 5, 2025
Base automatically changed from update-test-framework to develop February 25, 2025 20:55
@zorkow
Copy link
Member

zorkow commented Feb 26, 2025

  • In original line 98, since we know (from line 94) that we have mu units, that means value will not be an empty string, and so the || '1' is not needed.

I tried to work out why this holds. The real reason is that muReplace is only ever called from matchDimen where the current regexp guarantees that there is a numerical value before the unit. However, one could replace the num field in UNIT_CASES with a regexp that would allow for an empty value, which would lead to an error in the parseFloat expression. The following will do the trick:

UnitUtil.UNIT_CASES.num = '([-+]?([.,]\\d*|\\d*([.,]\\d*)?))'
UnitUtil.UNIT_CASES.set('mu', 1/18);
  • In original line 146, we know factor is defined, since the UNIT_CASES.get() function in the previous line will return the value for 'pt' (line 65) if the unit itself is not found. So unless someone deletes the 'pt' value, the factor should always be defined, and if it turns out to be 0, then factor * m will be 0, so there is no need for the test.

@dpvc
Copy link
Member Author

dpvc commented Feb 27, 2025

one could replace the num field in UNIT_CASES with a regexp that would allow for an empty value

Is that a reasonable thing to do? (That is, is this a use-case that we need to support?) Or would this regex be considered a mistake, and we would want something to go wrong so that the user knows it should be fixed? Because this is being used to determine when a dimension is the first (or only) thing in a string, and dimensions in TeX require numbers, it seems to me that such a regexp would lead to all kinds of trouble.

I'll change it and make a test for it if you want, but I just want to make sure that it is actually a reasonable regex to use for num.

Copy link
Member

@zorkow zorkow left a comment

Choose a reason for hiding this comment

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

as per f2f we don't worry about that case now.

@dpvc dpvc merged commit 2f36623 into develop Feb 27, 2025
@dpvc dpvc deleted the test-unitutil branch February 27, 2025 15:05
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