patchwork cleanups and enhacements #137
Conversation
Pull Request Test Coverage Report for Build 464
💛 - Coveralls |
sktm/patchwork.py
Outdated
| Get project ID based on its name. Child classes need to implement | ||
| this method. | ||
| """ | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Can I ask you to write full documentation for this method? I think it's even more important to document than the implementations, as it defines the interface.
sktm/__init__.py
Outdated
| @@ -124,7 +123,6 @@ def add_pw(self, baseurl, pname, lpatch=None, restapi=False, apikey=None, | |||
| baseurl, pname, int(lpatch) if lpatch else None, skip | |||
| ) | |||
|
|
|||
| # FIXME Figure out the last patch first, then create the interface | |||
There was a problem hiding this comment.
I didn't read this code now, but I think converting between project ID and name shouldn't be a job for a patchwork project class, but rather for a patchwork server (instance) class. The latter can have an interface to retrieve project objects for specific IDs and have an interface for conversion to and from a project name, i.e. the _get_project_id() function should be moved there. Then we will be able to deal with these FIXMEs properly.
That's not something we need to do in this PR, though.
There was a problem hiding this comment.
Having a class to deal with a full Patchwork server is definitely a neat idea. Do you think you can submit an issue for that, so we don't forget about it while dealing with other things?
| parser_patchwork.add_argument("--lastpatch", type=str, help="Last patch " | ||
| "(id for pw1; datetime for pw2)") | ||
| parser_patchwork.add_argument("--lastpatch", type=int, | ||
| help="Last patch ID") |
sktm/__init__.py
Outdated
| lpatch: Last processed patch. Patch ID, if adding an XML | ||
| RPC-based interface. Patch timestamp, if adding a | ||
| REST-based interface. Can be omitted to | ||
| lpatch: Last processed patch. Patch ID. Can be omitted to |
There was a problem hiding this comment.
Do you think saying ID of the last processed patch. here would read better?
| @@ -902,8 +901,7 @@ def get_patch_by_id(self, pid): | |||
| patch = self.rpc.patch_get(pid, self.fields) | |||
|
|
|||
| if patch is None or patch == {}: | |||
| logging.warning("Failed to get data for patch %d", pid) | |||
| patch = None | |||
| raise Exception('Can\'t get patch by id %d)'.format(pid)) | |||
Introduce a method in the parent class that the child classes are supposed to implement and override (fixes composition and pylint error) and modify the argument name of the Patchwork1Project class' method to match the one in the parent. Also fix the docstring about returned value which didn't match what was actually being returned. Signed-off-by: Veronika Kabatova <[email protected]>
It's assigned to None and never touched again. Just remove it. Signed-off-by: Veronika Kabatova <[email protected]>
Seems like a legacy format was used here. pylint-3 is not a fan. Signed-off-by: Veronika Kabatova <[email protected]>
We can't retrieve the data related to the project from the database without having the project ID, for which we need the Patchwork project interface. So figuring out the last patch before having the interface is not possible. Remove the FIXMEs suggesting to do it. Signed-off-by: Veronika Kabatova <[email protected]>
Fixes cki-project#46 Signed-off-by: Veronika Kabatova <[email protected]>
Throw an exception if the requested patch can't be retrieved. This was already done for PatchworkV2Project but not PatchworkV1Project. Being unable to retrieve a patch previously known as existing signals a possible problem in sktm's logic, and it should be handled as such. The unification also allows to remove the return value checks when this method is used. Signed-off-by: Veronika Kabatova <[email protected]>
|
@spbnick docs updated! |
|
Thank you, Veronika, and thank you for splitting the changes so nicely :) |
No description provided.