Skip to content
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

Refactor some code for understandability #130

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

micahjsmith
Copy link

@micahjsmith micahjsmith commented Dec 17, 2018

Hello,
I spent a fair amount of time reading through the code of jsonmodels to understand how it worked. It employs a fair amount of interesting python constructs, like __get__ and __set__ methods of BaseField and a healthy use of metaclasses and class methods.

To help myself understand what was going on, I found myself making small changes to the code for readability and interpretability. This includes

  • consistently use if/else to make control flow explicit
  • fix typo of structure_name (still leaving structue_name for backwards compatibility)
  • use more explicit names for iteration over class attributes
# original
for attr in dir(cls):
    clsattr = getattr(cls, attr)

# new
for attr_name in dir(cls):
    field = getattr(cls, attr_name)
  • try to simplify logic of field initialization by adding an _initialized attribute to BaseField and guarding against re-initialization
  • make Base.get_field a classmethod, which avoids a linear search through the entire set of fields on each access

I thought other people might benefit from these small changes. Hope this is helpful, let me know what you think!

@micahjsmith
Copy link
Author

Hi @beregond , any thoughts on this PR?

@micahjsmith
Copy link
Author

Hi @beregond any thoughts on this PR?

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.

1 participant