Skip to content

refactor(v4): split objects and managers per API resource #1288

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Feb 15, 2021
Merged

refactor(v4): split objects and managers per API resource #1288

merged 2 commits into from
Feb 15, 2021

Conversation

nejch
Copy link
Member

@nejch nejch commented Feb 7, 2021 •
edited
Loading

This one is huge, so sorry 🙈 It does what #997 was for (splitting the 6000 line monster), but with less restructuring (only splitting the resources into modules). I'm not sure if all the importing is the best approach, but it's quite explicit.

I can imagine this is too much for review so I can try to split it, but the managers were quite intertwined so I was fixing things along the way. Here is the new import chain (mostly due to Project and Group including many other managers) - output of pydeps --rmprefix='gitlab.v4.objects.' --only='gitlab.v4.objects' gitlab/v4/objects/__init__.py:

image

Each module only imports the managers it needs, while __init__.py does a star import of all of them for backwards compatibility. Each module exports only its own classes for the star import explicitly via __all__.

In the end this needed to happen in my opinion as well, to keep sane file sizes. But not sure what's more painful, reviewing this big PR or doing it in stages. I'm sure there are better ways to do this so if anyone has ideas, while keeping backwards compatibility, I'd be open too! :)

If this gets merged I can rebase other people's PR's touching objects so it's not too demotivating for them.

This would now roughly imitate the structure of the GitLab API docs and gitlab's own lib/api directory:

@nejch
Copy link
Member Author

nejch commented Feb 7, 2021 •
edited
Loading

Oh no 😁
image

"Ready for review"... 🤣

@nejch nejch marked this pull request as ready for review February 7, 2021 21:51
@nejch nejch requested a review from max-wittig February 7, 2021 21:51
@max-wittig
Copy link
Member

That's a nice MR 👍 Will indeed take some time to review 😄

@nejch
Copy link
Member Author

nejch commented Feb 11, 2021

That's a nice MR 👍 Will indeed take some time to review 😄

Understandable 😄 I found that with git diff --color-moved master, it should show with different colors what's moved and what's added/deleted (or git diff --color-moved=plain master since the default gives you a funky zebra pattern).

So it'd show it's mostly just cut/paste :) I couldn't really find a good GUI difftool that does it well for moves across files but I'm sure there's some out there, if it makes it a bit clearer.

@nejch nejch mentioned this pull request Feb 14, 2021
Closed
@max-wittig
Copy link
Member

Thanks for all the refactoring done here 👍

@max-wittig max-wittig merged commit 9fcd962 into master Feb 15, 2021
@max-wittig max-wittig deleted the refactor/split-objects branch February 15, 2021 07:48
@bufferoverflow
Copy link
Member

@nejch very nice work! @max-wittig a heavy review 😄

@JohnVillalovos
Copy link
Member

@nejch Any plans on doing something similar for gitlab/init.py ?

@nejch
Copy link
Member Author

nejch commented Feb 15, 2021 •
edited
Loading

@nejch Any plans on doing something similar for gitlab/init.py ?

I wasn't currently working on anything, but maybe it would make sense to take the class out into something like a client module to make it slim, and possibly split it a bit or if you have any other ideas. I think @max-wittig you also mentioned you wanted some cleanup there some time ago? 😀 But maybe you can open a new issue (or PR) for that.

@JohnVillalovos
Copy link
Member

@nejch Any plans on doing something similar for gitlab/init.py ?

I wasn't currently working on anything, but maybe it would make sense to take the class out into something like a client module to make it slim, and possibly split it a bit or if you have any other ideas. I think @max-wittig you also mentioned you wanted some cleanup there some time ago? 😀 But maybe you can open a new issue (or PR) for that.

Thanks. I'll try to do a PR and move most of what is in gitlab/__init__.py to gitlab/client.py.

@nejch nejch mentioned this pull request Feb 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@max-wittig max-wittig Awaiting requested review from max-wittig

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nejch @max-wittig @bufferoverflow @JohnVillalovos