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.

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

feat: merge holders with storage #1167

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from
Draft

feat: merge holders with storage #1167

wants to merge 24 commits into from

Conversation

bonjourmauko
Copy link
Member

@bonjourmauko bonjourmauko commented Nov 30, 2022 •
edited
Loading

Fixes #887
Depends on #1235

New features

  • Use UserDict to encapsulate the data model of the data_storage module.

Technical changes

  • Add tests to data_storage.
  • Add typing to data_storage.
  • Add documentation to data_storage.

@bonjourmauko bonjourmauko requested a review from a team November 30, 2022 18:50
@bonjourmauko bonjourmauko added level:starter Suited for beginners and new contributors kind:refactor Refactoring and code cleanup labels Nov 30, 2022
@openfisca openfisca deleted a comment from coveralls Nov 30, 2022
MattiSG
MattiSG reviewed Dec 1, 2022
View reviewed changes
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

This improves documentation significantly, seems to add interesting value checks, and adds types in a way that I don't feel confident checking.

I invoke #1159 pre-approval and am willing to try this changeset on the Country Template.

openfisca_core/data_storage/on_disk_storage.py Outdated Show resolved Hide resolved
@bonjourmauko bonjourmauko force-pushed the add-protocol-data-storage branch 2 times, most recently from 845e5ba to 8c125a9 Compare December 1, 2022 17:06
bonjourmauko
bonjourmauko commented Dec 6, 2022
View reviewed changes
openfisca_core/entities/entity.py Outdated
@@ -43,7 +49,7 @@ def check_variable_defined_for_entity(self, variable_name: str) -> None:
if variable is not None:
entity = variable.entity

if entity.key != self.key:
if self != entity:
Copy link
Member Author

Choose a reason for hiding this comment

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

@benjello here.

@bonjourmauko bonjourmauko changed the title Add typing to data storage module Document the data_storage module Dec 9, 2022
@bonjourmauko bonjourmauko force-pushed the add-protocol-data-storage branch 2 times, most recently from df7e6b4 to 4b5627a Compare December 9, 2022 19:36
@bonjourmauko bonjourmauko mentioned this pull request Dec 9, 2022
Open
17 tasks
@bonjourmauko bonjourmauko changed the title Document the data_storage module [2/17] Document the data_storage module Dec 9, 2022
@bonjourmauko bonjourmauko mentioned this pull request Dec 9, 2022
Open
17 tasks
@bonjourmauko
Copy link
Member Author

@benjello What do you think?

benjello
benjello reviewed Sep 25, 2023
View reviewed changes
openfisca_core/holders/holder.py

def get_memory_usage(self) -> MemoryUsage:
"""Get data about the virtual memory usage of the Holder.
"""Gets data about the virtual memory usage of the Holder.
Copy link
Member

Choose a reason for hiding this comment

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

I would use imperative here and in other places

@benjello
Copy link
Member

Hi @maukoquiroga : I am not sure I can review this PR since I am not qualified enough.
May be @maukoquiroga @sandcha @benoit-cty or @eraviart can give you a better review than myself.

If you point me to some specific problem where I can actually help, I would do it.

@bonjourmauko
Copy link
Member Author

bonjourmauko commented Sep 26, 2023 •
edited by benjello
Loading

Hi @maukoquiroga : I am not sure I can review this PR since I am not qualified enough. May be @maukoquiroga @sandcha @benoit-cty or @eraviart can give you a better review than myself.

If you point me to some specific problem where I can actually help, I would do it.

As it is, it is mainly documentation (moving the data storage is a breaking change, but can't be downplayed by doing a doublwe import).

However, I'd like to have your opinion in where this could go, maybe in a second pull request, or in this one:

  1. Packaging together Holder and Storage.
  2. Order storage to make it more easily extensible.

In principle, everything besides documentation (including types) and refactoring should be extracted into a second pull request, so this one can be patch or minor?

In terms of style, I'm trying everywhere to apply [[STYLEGUIDE.md]] (WIP).

@benoit-cty
Copy link
Contributor

Hello Mauko, Can you run OpenFisca-France tests with this version of Core ?

@bonjourmauko bonjourmauko changed the title [2/17] Document the data_storage module docs: document the data_storage module Sep 30, 2024
@bonjourmauko bonjourmauko marked this pull request as draft October 1, 2024 02:54
@bonjourmauko bonjourmauko changed the title docs: document the data_storage module feat: merge holders with storage Oct 4, 2024
@bonjourmauko bonjourmauko self-assigned this Oct 4, 2024
@bonjourmauko bonjourmauko added kind:feat A feature request, a feature deprecation and removed level:starter Suited for beginners and new contributors kind:refactor Refactoring and code cleanup labels Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjello benjello benjello left review comments

@MattiSG MattiSG Awaiting requested review from MattiSG

At least 1 approving review is required to merge this pull request.

Assignees

@bonjourmauko bonjourmauko

Labels
kind:feat A feature request, a feature deprecation
Projects
OpenFisca International
Status: Inbox
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide an explicit Cache API
4 participants
@bonjourmauko @benjello @benoit-cty @MattiSG