Skip to content

chore: add _create_attrs & _update_attrs to RESTManager #1371

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 14, 2021
Merged

chore: add _create_attrs & _update_attrs to RESTManager #1371

merged 1 commit into from
Mar 14, 2021

Conversation

JohnVillalovos
Copy link
Member

Add the attributes: _create_attrs and _update_attrs to the RESTManager
class. This is so that we stop using getattr() if we don't need to.

This also helps with type-hints being available for these attributes.

Add the attributes: _create_attrs and _update_attrs to the RESTManager
class. This is so that we stop using getattr() if we don't need to.

This also helps with type-hints being available for these attributes.
Copy link
Member

@nejch nejch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @JohnVillalovos sorry for the delay, thanks a lot!

I really liked the namedtuple approach in your earlier PR #1366, is the idea to introduce it back in a follow-up so this can be a smaller change for now?

It would take out some of the mystery in all those attributes for new contributors I think.

@JohnVillalovos
Copy link
Member Author

Hey @JohnVillalovos sorry for the delay, thanks a lot!

No worries! Thanks for reviewing 😀

I really liked the namedtuple approach in your earlier PR #1366, is the idea to introduce it back in a follow-up so this can be a smaller change for now?

You read my mind! I thought let's get the small change in first and wait on the giant change.

It would take out some of the mystery in all those attributes for new contributors I think.

Agreed. It took me awhile to figure out what they were for. I think with the NamedTuple it should be clearer.

Thanks again.

@JohnVillalovos
Copy link
Member Author

@nejch Once this is merged I will work on the follow-up patch to use namedtuple.

@nejch nejch merged commit 8603248 into python-gitlab:master Mar 14, 2021
@JohnVillalovos JohnVillalovos deleted the jlvillal/create_attrs_1 branch March 14, 2021 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants