-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
Make registry.Interface construction work with no standard_id. #506
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #506 +/- ##
=======================================
Coverage 80.11% 80.12%
=======================================
Files 52 52
Lines 6085 6087 +2
=======================================
+ Hits 4875 4877 +2
Misses 1210 1210 ☔ View full report in Codecov by Sentry. |
pyvo/registry/regtap.py
Outdated
self.is_vosi = (standard_id | ||
and standard_id.startswith("ivo://ivoa.net/std/vosi")) |
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.
Although I agree that there is unlikely to be a problem with self.is_vosi
sometimes being None
versus False
, I do think there is some value in consistency, and alternative code not too verbose still easy to read (as long as we don't use the "ternary operator").
self.is_vosi = (standard_id | |
and standard_id.startswith("ivo://ivoa.net/std/vosi")) | |
if standard_id is not None: | |
self.is_vosi = standard_id.startswith("ivo://ivoa.net/std/vosi") | |
else: | |
self.is_vosi = False |
As a secondary comment, I was considering whether to ask for covering this with a unit test and looked at the existing tests. It was curious to me why this test hasn't been raising exceptions when I realized that the modified statement above is using standard_id
the function arg as opposed to self.standard_id
the class member, so in the test standard_id
is not None
even though self.standard_id
is. So maybe a better suggestion would be this since it would then get tested for free with the existing test:
self.is_vosi = (standard_id | |
and standard_id.startswith("ivo://ivoa.net/std/vosi")) | |
if self.standard_id is not None: | |
self.is_vosi = self.standard_id.startswith("ivo://ivoa.net/std/vosi") | |
else: | |
self.is_vosi = False |
I agree that no change log is needed for this. |
Interface.__init__ so far tries a .startswith on something that can be None (actually, will be by virtue of the preceding line). This commit fixes this.
ab165ce
to
d24891a
Compare
On Fri, Dec 15, 2023 at 07:15:11AM -0800, Tom wrote:
@tomdonaldson commented on this pull request.
there is some value in consistency, and alternative code not too
verbose still easy to read (as long as we don't use the "ternary
Fair enough. I've force-pushed something that takes your suggestion.
Do you let me get away without a changlog entry? To me, it seems a
bit too trivial to mention, and it seems it's not bitten anyone yet.
|
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 @msdemlei. This looks good to me.
Interface.init so far tries a .startswith on something that can be None (actually, will be by virtue of the preceding line). This commit fixes this (at the cost of is_vosi being variously False or None, which should be operationally insignificant for now).
This is a really tiny fix -- perhaps we can sneak it in without a changelog entry?