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.
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
[MRG] IS float #1720
[MRG] IS float #1720
Changes from all commits
8c6ad1d
d1fb0a9
5dcda34
80d8d3c
7e09eac
cb29fc4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allowing ISfloat by default caused some json tests to fail. Added this strict context to try to keep previous behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some trouble understanding the logic here: if
suppress_invalid_tags
is set, we raise an exception, and will be cught and ignored here? I'll probably have another look tomorrow with a fresh brain...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the failing test (
test_suppress_invalid_tags_with_failed_dataelement
) - I wrote that, and I think it just should be changed to use another invalid value that would still raise, for example:and revert the change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that too, but that would still change existing behavior for IS - ones that did not get written before would now be accepted and written out. Probably not a big deal, but still something a bit different. I'm not sure just skipping values in the output is the best idea anyway. I'll think about this a bit before this is merged. I'd still like to have a "bigger picture" in mind for how to handle settings in v3.X... I'll start an issue and post some thoughts on that in the coming days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true, but I assumed that this is a change we want to make. Though maybe I haven't thought this through... I will wait your thoughts then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a look at this again ... This change has no effect (
nullcontext()
) for the default case wheresuppress_invalid_tags
is False. So I think I was right, ifsuppress...
is True (as seen only in tests probably at the moment), then we will catch the exception and ignore it, as is currently done (the newISFloat
is not allowed withstrict_reading()
turned on).It is still messy, but I'd rather push real fixes to this messy config (and all the other messy config) to v3.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I leave the decision to you. I find it a bit ugly, but you are probably right - better defer the real fix to 3.0, where behavior changes are expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly necessary to go back to binary like this rather than just creating an
IS
, but I thought it a good idea in case of future hooks into reading invalid values.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit odd to me to have two different Errors for the same value, but that is the way it is done currently. Perhaps to be revisited for the grand redesign of validation settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember that I was not happy about this either, though forgot why I didn't change it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In part I guess there are two branches, so to speak, because some are by string processing (regex) and others are by value. Another thing to think about in harmonizing validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recently learned that
black
uses 88 instead of 79 characters as a default maximum line length, and I tend to agree with it... I think we had this discussion before, and it is not really important, though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to raise this point as well. I find 79 very limiting, it would really help to have even 9 more characters, and useful to just let
black
do its thing on new code before posting a PR.We can change to 88 in a separate PR maybe, and update the documentation for contributors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, of course - this was not meant for this PR, just wanted to mention it.