Repository navigation
refactor: decouple CLI from custom method arguments - #1999
nejch wants to merge 1 commit into
Conversation
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## main #1999 +/- ##
==========================================
+ Coverage 94.62% 94.68% +0.05%
==========================================
Files 78 78
Lines 4983 5038 +55
==========================================
+ Hits 4715 4770 +55
Misses 268 268
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Sorry, something went wrong.
|
Incidentally this makes a few of the decorators a bit more readable.. and should be much more readable when I get to removing the @cli.register_custom_action somehow, to completely get rid of the CLI from the API client itself. |
Sorry, something went wrong.
JohnVillalovos
left a comment
There was a problem hiding this comment.
Sorry for the short review. I haven't yet read the whole PR but wanted to leave a couple comments.
Sorry, something went wrong.
|
@JohnVillalovos this should be ready for another round I think. If the value of this PR isn't really clear I can add a draft PR built on top of it, where I start removing custom argument parsing from some of the custom methods to start demoing that, similar to what I did in https://github.com/python-gitlab/python-gitlab/pull/1985/files. |
Sorry, something went wrong.
| return result | ||
|
|
||
| @cli.register_custom_action(("ProjectIssue", "ProjectMergeRequest"), ("duration",)) | ||
| @cli.register_custom_action(("ProjectIssue", "ProjectMergeRequest")) |
There was a problem hiding this comment.
What happens if these two decorators are put in reverse order?
Does it work? My guess is no. If not, do we detect a problem? My guess is also no.
If my guesses are correct can we add a way to detect improper usage and make sure the unit tests or something else fails?
Sorry, something went wrong.
There was a problem hiding this comment.
Good question.. the thing is that, as I said in the comment above #1999 (comment), @cli.register_custom_action is the very next thing we'd want to get rid of if we want to fully decouple the CLI from the client. :( So not sure we should invest time in that and then remove it right after. 🤔
My idea for that was, instead of manually assigning custom methods to classes at each method definition, simply collect all non-standard (create,..) public methods for each Manager and RESTObject in the CLI module, kind of how we already collect the classes for the CLI:
https://github.com/python-gitlab/python-gitlab/blob/main/gitlab/cli.py#L97
Let me check if I can do that in a clean way in a separate commit or follow-up PR, I'll mark this a draft until then.
Maybe it makes sense I open an issue with a mini roadmap to untangling the CLI from the API so we have clear steps on how to get there.
Sorry, something went wrong.
|
Thanks @nejch Some quick comments Can the commit message be improved? It seems to only have a subject line. Am I correct in thinking (based on looking at https://github.com/python-gitlab/python-gitlab/pull/1985/files) that the _custom_attrs will be used to determine which values should be sent in the POST body versus in the URL string? If I am correct, would it make more sense to figure out what can not be in the POST body and put everything that can be in the POST body by default? For example sudo is something that has to be in the query-string (or header) but not in the POST body. Maybe there are a few others like that?? |
Sorry, something went wrong.
|
Closing this to reduce the number of drafts open - I'll keep the branch and reopen when ready. |
Sorry, something went wrong.
A more generic approach for #1985
This helps towards #1982. This way, instead of handling parameters in every individual custom method, we can add them in a more declarative way in the decorators and just pull payload in POST/PUT methods from that.
It will also help us completely decouple the CLI from the API client hopefully - making it easier to switch libraries or approaches if we can untangle this.