-
Notifications
You must be signed in to change notification settings - Fork 160
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
Improve error handling for Image
, Images
, PreImage
, PreImages
#3454
Improve error handling for Image
, Images
, PreImage
, PreImages
#3454
Conversation
a5210d2
to
7fa0e7e
Compare
Codecov Report
@@ Coverage Diff @@
## master #3454 +/- ##
==========================================
+ Coverage 80.47% 85.35% +4.88%
==========================================
Files 696 699 +3
Lines 343497 346557 +3060
==========================================
+ Hits 276413 295797 +19384
+ Misses 67084 50760 -16324
|
Could you write tests to hit all these new cases, just to sanity check them? |
Image
, Images
, PreImage
, PreImages
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 am not convinced this is correct, see my inline comment. I guess my claim on what worked before and now doesn't work should be verified. Also, as @ChrisJefferson suggested, tests would be really nice.
I'll definitely set up some tests as @ChrisJefferson suggested and have a closer look at your inline comment @fingolfin. |
7fa0e7e
to
afa8ae0
Compare
I'm updating the PR right now. I have added tests and deleted the
lines. This "else" branch can't be executed since the function compares the families of the source and of the argument |
bb96e86
to
8064400
Compare
PR is updated. |
9e63abd
to
67fb345
Compare
And again some HPCGAP tests broke. Let's see whether it works now. |
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 principle this seem to be on a good way, just some minor nitpicking
67fb345
to
1e746b2
Compare
I've addressed your remarks. |
1e746b2
to
070dfae
Compare
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 requests concerning the error messages, particular about the use of the name <coll>
.
I adopted the suggested improvements. I put the changes into a separate commit so you can see more easily what I've changed. I'll turn the new commits into a single one after the PR is accepted. |
1764792
to
21d7375
Compare
.. and I rebased onto master. |
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 for your changes.
..., `Images`, `PreImage`, and `PreImages`. If one of these functions figures out that the second argument is not an element or subset of the Range or the Source, it now tells the user. The same changes are replicated to `hpcgap/lib/mapping.gi`. Improves the structuring of the `mapping.tst` file. Adds tests for the new error handling. Also adds some general tests for `Image`, `Images`, `PreImage`, `PreImages`.
21d7375
to
b4ab5d0
Compare
Squashed. |
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.
Thank you!
..., PreImage, and PreImages.
If one of these functions figures out that the second argument is not an
element or subset of the Range or the Source, it now tells the user.