- Notifications
You must be signed in to change notification settings - Fork675
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
chore: add _create_attrs & _update_attrs to RESTManager#1371
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Add the attributes: _create_attrs and _update_attrs to the RESTManagerclass. 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.
nejch left a comment
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.
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 commentedMar 13, 2021
No worries! Thanks for reviewing 😀
You read my mind! I thought let's get the small change in first and wait on the giant change.
Agreed. It took me awhile to figure out what they were for. I think with the NamedTuple it should be clearer. Thanks again. |
JohnVillalovos commentedMar 13, 2021
@nejch Once this is merged I will work on the follow-up patch to use namedtuple. |
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.