Remove Admin and move its methods to JobManager #349
Conversation
tests/conftest.py
Outdated
| def serial(): | ||
| return itertools.count(1) | ||
|
|
||
|
|
There was a problem hiding this comment.
Why did you remove this? The job_factory fixture still uses it…
There was a problem hiding this comment.
I wonder if it wouldn't make more sense to make an undefered job, and then let the manager defer it. The InMemory backend already maintains a counter.
(Anyway, WIP :D )
There was a problem hiding this comment.
I see. But with this the object provided by the new job_factory fixture (that you'd named deferred_job_factory) would a coroutine function. So that would imply doing job = await job_factory(…) instead of job = job_factory(…) everywhere we use that fixture. That's a lot of changes. I'd suggest that, if we want to do that, we do it with a separate PR.
tests/conftest.py
Outdated
|
|
||
| return factory | ||
|
|
||
|
|
There was a problem hiding this comment.
I also don't see the reason for this new fixture. It's not used currently.
procrastinate/manager.py
Outdated
| for row in await self.connector.execute_query_all_async( | ||
| query=sql.queries["list_tasks"], | ||
| queue_name=queue, | ||
| name=task, |
There was a problem hiding this comment.
It should be task_name=task here.
|
I added a WIP3 commit (a wommit in our new jargon :) that fixes the tests. |
Codecov Report
@@ Coverage Diff @@
## master #349 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 29 28 -1
Lines 1575 1568 -7
Branches 172 171 -1
=========================================
- Hits 1575 1568 -7
Continue to review full report at Codecov.
|
elemoine
force-pushed
the
admin-333
branch
2 times, most recently
from
cbf7485 to
bf13f0b
Compare
elemoine
changed the title
Remove the Admin class and move its methods to JobManager. Co-authored-by: Éric Lemoine <[email protected]>
elemoine
requested review from
CorBott,
DainDwarf,
mgu,
pmourlanne and
sophie-ulti
as code owners
|
I fixed the tests, and improved the doc strings making sure that the generated API reference looks good. I think this is ready for merging. |
Closes #333
Successful PR Checklist: