-
Notifications
You must be signed in to change notification settings - Fork 10
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
Added fix for adding annotations that are not strings in ontodoc #512
Added fix for adding annotations that are not strings in ontodoc #512
Conversation
Furthermore, reenabled test for ontodoc. This is still aminimal test, but I think can be gradually expanded. Lastlt - because a new altLabel was added in the testontology, it was revieled that the same entity is returned twice in by get_by_label_all in test_prefix. Issue #511 was made bacause of this.
Codecov Report
@@ Coverage Diff @@
## master #512 +/- ##
==========================================
+ Coverage 63.08% 65.46% +2.38%
==========================================
Files 16 16
Lines 3080 3081 +1
==========================================
+ Hits 1943 2017 +74
+ Misses 1137 1064 -73
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -288,6 +288,7 @@ def itemdoc( | |||
annotations.keys(), key=lambda key: order.get(key, key) | |||
): | |||
for value in annotations[key]: | |||
value = str(value) |
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 could fail, I suppose, if the type of value
cannot be cast to a str
type.
You could instead wrap it in a try/except
block, where you're catching TypeError
and ValueError
exceptions, re-raising them with an appropriate custom message?
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 discussed this with @jesper-friis , and we can't really see a situation in which this not enough. I think it is better to leave it as is, and update this if and when we find examples of situations in which a different handling is needed.
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.
As you wish. Casting to str
is in general quite generous in this case, so it's fine I suppose. However, I'd argue it's good convention to consider possible edge cases and try to catch them if possible so exception messages are clear and helpful.
Furthermore, reenabled test for ontodoc. This is still a minimal test, but I think it can be gradually expanded.
Lastly - because a new altLabel was added in the testontology, it was reveiled that the same entity is returned twice in get_by_label_all in test_prefix. Issue #511 was made bacause of this.
Description:
Type of change:
Checklist:
This checklist can be used as a help for the reviewer.
Comments: