NAS-135049 / 25.10 / Support new-style jobs #30
Conversation
bugclerk
changed the title
themylogin
force-pushed
the
NAS-135049
branch
7 times, most recently
from
a7f9bde to
3951ee1
Compare
There was a problem hiding this comment.
I don't see CI tests for new_style in the middleware companion PR.
Not directly part of this PR, but on line 58 ErrnoMixin is imported but not used.
Also....
Not that I particularly care, but Flake8 does and it's specified in pep8. https://peps.python.org/pep-0008/#whitespace-in-expressions-and-statements
When combining an argument annotation with a default value, however, do use spaces around the = sign
I'm not requiring any change, just commenting.
What's
It's used by those who do
What line are you referring to? |
Sorry, typo
OK, thanks for clearing that up. 👍
This is flake8 religiously following pep8 and not my personal preference. |
Sorry, still don't understand what are you talking about? What needs to be tested? |
|
@mgrimesix i fixed existing PEP8 issues in another PR |
| if self._new_style_jobs: | ||
| # With new-style jobs, the method return value will not be sent until the job is completed | ||
| # We expect just to receive job ID immediately, so let's set call return value to the | ||
| # newly added job ID. | ||
| if params['collection'] == 'core.get_jobs' and params['msg'] in ['added', 'changed']: | ||
| for message_id in params['fields']['message_ids']: | ||
| if (call := self._calls.get(message_id)) is not None: | ||
| call.result = params['id'] | ||
| call.returned.set() | ||
| self._unregister_call(call) |
There was a problem hiding this comment.
Sorry, still don't understand what are you talking about? What needs to be tested?
Testing the above path.
Let me know if the existing set of tests already cover it. Maybe point one out?
There was a problem hiding this comment.
@mgrimesix new middleware uses new-style jobs, so any test that runs a job tests the above path.
|
This PR has been merged and conversations have been locked. |
No description provided.