-
Notifications
You must be signed in to change notification settings - Fork 0
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
Allow comparison of SpectralType directly to string #45
Conversation
If a string represents a valid constructor, it can be compare directly to an instance of SpectralType, which can in some cases be a lot more convenient than having to manually convert the string to a SpectralType. Implementing the gt and ge methods was necessary to also allow reverse comparisons, i.e. `"B0V" < SpectralType("A0V")`.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #45 +/- ##
==========================================
+ Coverage 99.28% 99.30% +0.02%
==========================================
Files 6 6
Lines 420 434 +14
==========================================
+ Hits 417 431 +14
Misses 3 3 ☔ View full report in Codecov by Sentry. |
@classmethod | ||
def _comp_guard(cls, other): | ||
if isinstance(other, str): | ||
other = cls(other) | ||
if not isinstance(other, cls): | ||
raise TypeError("Can only compare equal types or valid str.") | ||
return other |
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.
If you'd rewrite this function as
@classmethod | |
def _comp_guard(cls, other): | |
if isinstance(other, str): | |
other = cls(other) | |
if not isinstance(other, cls): | |
raise TypeError("Can only compare equal types or valid str.") | |
return other | |
@classmethod | |
def _comp_guard(cls, other): | |
if isinstance(other, cls): | |
return cls | |
return cls(other) |
then it is even less SpectralType specific. Wouldn't there be something like this already somewhere? I don't know of anything 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.
Yeah, this is probably simpler and thus better. Any other type business will be caught by the constructor anyway. Although I'm a bit skeptical about your return cls
there 😉 (had to read it four times though to see that)
Maybe even shorter:
return other if isinstance(other, cls) else cls(other)
Fits well within one line 😄
Wouldn't there be something like this already somewhere?
IDK, let me know if you find something ^^
If a string represents a valid constructor, it can be compare directly to an instance of SpectralType, which can in some cases be a lot more convenient than having to manually convert the string to a SpectralType.
Implementing the gt and ge methods was necessary to also allow reverse comparisons, i.e.
"B0V" < SpectralType("A0V")
.