refactor: decouple CLI from custom method arguments#1999
Conversation
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.
|
|
Incidentally this makes a few of the decorators a bit more readable.. and should be much more readable when I get to removing the |
JohnVillalovos
left a comment
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.
nejch
force-pushed
the
refactor/decouple-cli-from-custom-args
branch
from
c8d6dcc to
8d3de21
Compare
nejch
force-pushed
the
refactor/decouple-cli-from-custom-args
branch
from
8d3de21 to
4561157
Compare
nejch
force-pushed
the
refactor/decouple-cli-from-custom-args
branch
from
4561157 to
5628004
Compare
|
@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. |
| 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?
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.
|
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 |
|
Closing this to reduce the number of drafts open - I'll keep the branch and reopen when ready. |
3 participants
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.