- Notifications
You must be signed in to change notification settings - Fork676
refactor: decouple CLI from custom method arguments#1999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-commenter commentedMay 1, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry for the short review. I haven't yet read the whole PR but wanted to leave a couple comments.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
c8d6dcc to8d3de21Compare8d3de21 to4561157Compare4561157 to5628004Compare@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 inhttps://github.com/python-gitlab/python-gitlab/pull/1985/files. |
| returnresult | ||
| @cli.register_custom_action(("ProjectIssue","ProjectMergeRequest"), ("duration",)) | ||
| @cli.register_custom_action(("ProjectIssue","ProjectMergeRequest")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
JohnVillalovos commentedJun 6, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 athttps://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. |
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.