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

Added a dict wrapper that acts like an object #1824

Merged
merged 2 commits into from
Dec 20, 2024
Merged

Conversation

dsblank
Copy link
Member

@dsblank dsblank commented Dec 11, 2024

@Nick-Hall had an interesting idea to make the raw dict from JSON act like an object. In that way, instead of writing:

if data["gender"] == Person.MALE:

you could write:

if data.gender == Person.MALE:

And if you rename data to person it looks like regular gramps for all of the attributes of all of the Gramps objects:

if person.gender == Person.MALE:

This has all of the benefits of quick access (no object creation) with a convenient syntax.

However that could create some confusion because methods can't be called as it isn't a real Person. With a couple of additional lines, it can act as a real object:

if person.get_gender() == Person.MALE:

This allows us to use the full object attribute and methods, only creating an object when necessary. Sometimes it is necessary, such as:

person.person_ref_list[0].get_referenced_handles()

(Although this only instantiates a PersonRef(), much cheaper than instantiating the whole Person)

With this PR applied, I found that person.get_gender() was 4.4 times slower than person.gender. The only downside with this PR is that those writing code (filter rules, for example) might not realize that they are creating an object when it is not necessary.

@dsblank
Copy link
Member Author

dsblank commented Dec 11, 2024

@stevenyoungs no data leakage! :)

@dsblank dsblank self-assigned this Dec 11, 2024
@dsblank
Copy link
Member Author

dsblank commented Dec 13, 2024

@Nick-Hall, some more decisions to make:

  1. Do we want to allow attribute syntax (data.gender) instead of dictionary syntax (data["gender"])? I think it is a nice enhancement, and is a very minimal change and cost.
  2. Do we want to allow method-calling (data.get_gender())? Although it hides object creation, it eliminates confusion, is sometimes needed, and is easy to implement.
  3. Shall we go with this implementation? I can't think of a better method to accomplish the above.

Let me know what you think, and I'll go with the decision on the next set of changes.

@Nick-Hall
Copy link
Member

@dsblank I'm happy with all of this. The attribute syntax is neat and concise, method-calling could be useful, and I don't have any objections to your implementation.

I was waiting for other comments before adding my opinion.

@dsblank
Copy link
Member Author

dsblank commented Dec 14, 2024

I'll work on adding tests, and prepare for moving from draft status.

@dsblank dsblank marked this pull request as ready for review December 19, 2024 14:16
@dsblank
Copy link
Member Author

dsblank commented Dec 19, 2024

Tests added, ready for review.

@Nick-Hall Nick-Hall force-pushed the dsb/raw-data-as-object branch from de3e0a8 to 93895a0 Compare December 20, 2024 21:32
@Nick-Hall Nick-Hall merged commit 8c3a9d3 into master Dec 20, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants