(NFC) Comment clarification in test class #17133
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I just updated the comments on this helper to clarify the limitations of the function & the fact that it should not be our only way to test thousand separators. I was noticing perfect was becoming the enemy of the good here. The function was marked as deprecated because it doesn't cover all scenarios - but the upshot was that we stopped increasing out thousand separator testing. In fact we need lots of form tests to do some testing of the separators and a very small number to test more variants - this latter has been added & the comments point to the need for more without going as far as deprecating
|
(Standard links)
|
|
nice |
totten
changed the title
|
It's basically NFC and @jaapjansma agrees with it, so merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
I just updated the comments on this helper to clarify the limitations of the function & the
fact that it should not be our only way to test thousand separators.
Before
Our main function for ensuring forms are passing values through clean money is marked as deprecated
After
Deprecation marker removed. Comments added to note the limitations of this function and that we also need to have some testing to ensure the clean function handles other currencies.
Technical Details
I was noticing perfect was becoming the enemy of the good here. The function was marked as deprecated because it doesn't cover all scenarios - but the upshot was that we stopped increasing out thousand separator testing.
In fact we need lots of form tests to do some testing of the separators and a very small number to test more variants - this latter has been added
& the comments point to the need for more without going as far as deprecating
Comments
@jaapjansma FYI