Fixing jobs UnknownElement warning #332
Conversation
Codecov Report
@@ Coverage Diff @@
## main #332 +/- ##
=======================================
Coverage 75.52% 75.53%
=======================================
Files 44 44
Lines 5131 5137 +6
=======================================
+ Hits 3875 3880 +5
- Misses 1256 1257 +1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
pyvo/io/uws/__init__.py
Outdated
| __all__ = ['parse_job', 'parse_job_list', 'JobFile', 'JobList'] | ||
| __all__ = ['parse_job', 'parse_job_list', 'JobFile', 'Jobs'] |
There was a problem hiding this comment.
Technically this is a public API change, and as such, it requires deprecation for the rename.
However, for some reasons, it's never ended up in public API docs, it was never added into the __all__ of its actual namespace.
So, overall I think it's OK to make this change without deprecation, BUT do list Jobs in the API here only if you also add it to __all__ in tree.py, otherwise the situation about the public API intent stays somewhat confusing.
There was a problem hiding this comment.
Good point. I don't think about it because I thought that JobList/Jobs is not meant to be part of the API as parse_job_list is and this function returns just the list of jobs and not the container class. To be technically correct it's probably simpler keep the JobList name but I agree that it needs to be added to the __all__ in tree.py.
There was a problem hiding this comment.
It's OK not being in the public API, but then remove from the __all__ in this file. I'm just basically advocating for consistency (and pretty sure that some tools should be out there that would warn about this undefined Jobs/Joblists here, in fact surprised that it didn't generate a sphinx warning).
There was a problem hiding this comment.
Another solution could be to rename it but make JobList point to Jobs in case someone is using it by that name.
There was a problem hiding this comment.
The naming of things is one thing, normally you would need a rename deprecation. But this was a bug all along, so I think we don't need it here.
However, whether deprecating or not, it still doesn't solve the issue with the namespace, Jobs is listed in __all__, yet it's not importable.
In [1]: from pyvo.io.uws import Jobs
---------------------------------------------------------------------------
ImportError Traceback (most recent call last)
<ipython-input-1-c5ca393952e0> in <module>
----> 1 from pyvo.io.uws import Jobs
ImportError: cannot import name 'Jobs' from 'pyvo.io.uws' (/Users/bsipocz/munka/devel/pyvo/pyvo/io/uws/__init__.py)
|
On Tue, May 31, 2022 at 11:06:52AM -0700, Brigitta Sipőcz wrote:
```
In [1]: from pyvo.io.uws import Jobs
---------------------------------------------------------------------------
ImportError Traceback (most recent call last)
<ipython-input-1-c5ca393952e0> in <module>
----> 1 from pyvo.io.uws import Jobs
ImportError: cannot import name 'Jobs' from 'pyvo.io.uws' (/Users/bsipocz/munka/devel/pyvo/pyvo/io/uws/__init__.py)
```
Ah... Good point. I don't think anyone has a reason to construct
JobList/Jobs objects from the outside, and so I've just removed the
export of Jobs. I've taken the liberty of amending and force-pushing
the commit (and now regret it, because it makes this discussion hard
to follow later -- aw, dang).
On the deprecation issue: I'd not be strongly against calling that
class JobList again; it just seemed a bit more consistent with the
rest of pyvo to call it after the element it is a facade for rather
than the endpoint it is retrieved from. Let me know if you prefer
that.
Pragmatically, I think nobody has or had a reason to muck around with
JobList and hence believe it's a harmless change taking off a wee bit
of technical debt. But I've been wrong on such assessments before.
|
This is an alternative to PR #331 to avoid the lone JobList class.