Repository navigation
BUG: avoid assert statements in business logic (io.ascii) - #17477
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
4fec8ca to
4e6db10
Compare
|
Note that there are still 4 failing tests with but they all have the same, orthogonal failure mode: astropy/astropy/io/registry/interface.py Line 56 in 7c800f8 I don't see an easy way out other than skipping these tests under optimized mode; the alternative being to refactor every method whose __doc__ attribute can be hit there and move the docstring to another attribute (the docstring would still be aliased to it, of course); this would be a substantial effort (probably not worth the expected gain), and wouldn't even work for external subclasses.
|
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Good to get rid of the functional asserts, but may as well aim to make the code clearer...
| @@ -1075,7 +1075,8 @@ def _validate_and_copy(col, converters): | |||
| # The input `converters` must be a ``type`` value that can init np.dtype. | |||
| try: | |||
There was a problem hiding this comment.
Can we just do
if not isinstance(converters, type):
try:
converters = [np.dtype(converters)]
except TypeError:
pass
There was a problem hiding this comment.
Taking this suggestion directly doesn't work (2 tests failures), but you're right that this logic can be simplified. I'll push what I think is the correct version of what you're proposing
| try: | ||
| new_val = cls(val) | ||
| assert new_val == val | ||
| if not new_val == val: |
There was a problem hiding this comment.
Here, I'd replace the assert with ok = new_val == val in the else clause.
There was a problem hiding this comment.
much better, thanks !
4e6db10 to
e70f979
Compare
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Ah, I guess the np.dtype was meant for things like float, np.int64 etc.
Looks good to me now - this obviously does the same as what was there before, so let's get it in.
Description
ref #17472