-
Notifications
You must be signed in to change notification settings - Fork 70
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
fix parser #522
fix parser #522
Conversation
@lbenet this fixes almost all errors in the ITF1788 files (except for reverse functions). The remaining ones are not related to parsing but to the DecoratedInterval constructor and hence it might be better to address that in a separate PR (decorated intervals constructor are my immediate next step on my todo list). |
Codecov Report
@@ Coverage Diff @@
## 1.0-dev #522 +/- ##
==========================================
Coverage ? 84.50%
==========================================
Files ? 34
Lines ? 1775
Branches ? 0
==========================================
Hits ? 1500
Misses ? 275
Partials ? 0 Continue to review full report at Codecov.
|
ah also to note that atm the tests are not checking that warnings are thrown when they should. I would like to address warnings checks in a separate PR (after the constructors are fixed) |
last but not least, if you are wondering where the green checks come from, that's because I temporarily disabled the tests for reverse functions... we know they are failing and there's an issue in IntervalContractors.jl, so better to leave those out for the time being. |
@lbenet now ALL tests related to strings / parsing pass 🎉 FYI the only failing ITF tests (except for reverse functions)
|
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.
It is great that the tests now pass =)
My comments are only about exception handling and documentation, all the rest looks good to me.
Co-authored-by: Benoît Richard <kolaru@hotmail.com>
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.
Thanks @lucaferranti for addressing this! In general, LGTM, and I'm in favor of merging this.
I left few comments that are optional. Perhaps the only relevant is related to the tests, which instead of being removed could be adapted to test other numeric formats (Float32, etc).
@lbenet now added some tests for |
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.
LGTM! Thanks a lot!
Merged! It's really nice to see again green colors in the tests! |
No description provided.