refactor: Centralize logic for normalizing strings in parser classes #1289
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
|
benmccann
reviewed
Apr 9, 2025
| if (ignoreZeroDecimal) { | ||
| s = StringUtils.removeZeroDecimal(s); | ||
| str = StringUtils.removeZeroDecimal(str); |
There was a problem hiding this comment.
reassigning a variable causes a warning in codacy
There was a problem hiding this comment.
Yep I noticed. I was thinking the code was like this to begin with and it is easy to end up with a growing task when starting to fix issues not introduced by the current PR. It is a also questionable IMHO that reassigning variables is bad practice. I am happy to change it @benmccann .
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.
Thanks for contributing.
Description
Centralizes logic for normalizing (pre-processing) strings as part of column type parsing.
Note this is mostly my own code from 5 years ago.
Testing
Existing test coverage is sufficient.
The Codacy warnings are questionable IMHO.
A review would be greatly appreciated @lwhite1 @benmccann.