Skip to content

Render the library more modular for contributors' goodness ✨#992

Merged
bonjourmauko merged 22 commits intomasterfrom
modularize
Apr 6, 2021
Merged

Render the library more modular for contributors' goodness ✨#992
bonjourmauko merged 22 commits intomasterfrom
modularize

Conversation

@bonjourmauko
Copy link
Copy Markdown
Member

@bonjourmauko bonjourmauko commented Apr 1, 2021

@openfisca/international-contrib what do you think?

Technical improvements

  • Render all OpenFisca Core components modular and sandboxed
    • Allows for simpler contribution
    • Allows for better unit testing
    • Allows for dealing with circular dependencies
    • Allows for more explicit contracts between components

Deprecations

  • Reorganise formula_helpers, memory_config, rates, simulation_builder.
    • Their functionalities are still there, just moved around
    • Transitional __init__.py files added to make transition smooth

@bonjourmauko bonjourmauko added the kind:refactor Refactoring and code cleanup label Apr 1, 2021
@bonjourmauko bonjourmauko requested a review from a team April 1, 2021 16:18
@augusto-herrmann
Copy link
Copy Markdown
Member

I think this proposal seems to make sense, especially considering, as you mentioned, it would avoid cyclic dependency problems.

Unfortunately I can't say anything from a practical point of view, as I am not really using OpenFIsca yet.

Copy link
Copy Markdown
Member

@fpagnoux fpagnoux left a comment

Choose a reason for hiding this comment

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

900 lines of code per file... phew 😅

Assuming this PR only moves things around without changing any functionality (i didn't look in details), I think leaning towards smaller and more modular files is a great idea. Thanks for the busy work!

@bonjourmauko
Copy link
Copy Markdown
Member Author

900 lines of code per file... phew 😅

Assuming this PR only moves things around without changing any functionality (i didn't look in details), I think leaning towards smaller and more modular files is a great idea. Thanks for the busy work!

Yes! I took care of keeping exposed all the same functions, classes, and modules, with the original names. I didn't expose protected functions however (the one of the ones _like(this):) but if people are relying on them we can just make them public.

That's basically why I didn't touch the tests at all.

@bonjourmauko bonjourmauko merged commit 219bc88 into master Apr 6, 2021
@bonjourmauko bonjourmauko deleted the modularize branch April 6, 2021 00:03
@MattiSG
Copy link
Copy Markdown
Member

MattiSG commented Apr 6, 2021

Wow, that's impressive! Fingers crossed that such a large changeset passes everywhere 🤞 If it all goes smoothly, I expect this to improve contributors’ navigation in the codebase 👏

@bonjourmauko
Copy link
Copy Markdown
Member Author

Thanks @MattiSG ! I plan to broadcast this and reach as many users as possible for help and feedback on possible issues. And yes I think this is a big step on making it easier for contributors to navigate and to contribute :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:refactor Refactoring and code cleanup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants