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

Unify syntax, add style checks, remove obsolete Python 2 leftovers #1047

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

Conversation

bonjourmauko
Copy link
Member

@bonjourmauko bonjourmauko commented Sep 28, 2021

Superset of openfisca/country-template#97
Follows discussion on #1033
Supersedes #977
Supersedes #960
Depends on #1160

Technical changes

  • Document/honestify:
    • Styles as enforced and skipped in setup.cfg
    • Actual dependecy versions whith whom OpenFisca runs in py37-39 (note that
      this library does not run with py36, py310, numpy v1.17, etc., which is not
      product of any particular change introduced by this changeset)
  • Make style checks stricter and clearer to help developers contribute:
    • Early returns are prefered for readability and performance
      • Before:
        if 1 > n:
            var = something
        else:
            var = something_else
        
        return var
      • After:
        if 1 > n:
            return something
        
        return something_elses
    • Normalise the use of:
      • Quotes is normalised to double-quotes
      • @staticmethod when self is unused
      • except from for better exception context
      • isinstance() as per the best practice
      • f-strings as per good practice
    • Favour:
      • ... as a placeholder and pass for loops
      • Relative to absolute imports to avoid circularity
      • Import sorting as described in the STYLEGUIDE
    • Avoid:
      • Catch-all exceptions in favour of more specific ones
      • Import aliases (so np becomes numpy)
    • Remove:
      • Code using exec because dangerous and hard to maintain
    • Extract:
      • Common imports to commons.imports
  • Most of these can be:
    • Corrected automatically running make format-style
    • Fixed by hand with the output of make test and make style-check

New features

  • Rename and makes the following public (note this are not considered as
    breking changes because methods were private, so renaming them to make
    them public counts as actually adding a new publicly exposed method, while
    removing a private method doesn't count as a breaking change):
    • Holder._to_array to Holder.to_array
    • Holder._set to Holder.put
    • Holder._memory_storage to Holder.memory_storage
    • parameters._compose_name to parameters.compose_name
    • parameters._validate_parameter to parameters.validate_parameter
    • ParameterNodeAtInstant._name to ParameterNodeAtInstant.name
    • ParameterNodeAtInstant._instant_str to ParameterNodeAtInstant.instant_str
    • ParameterNodeAtInstant._children to ParameterNodeAtInstant.children
    • Population._holders to Population.holders
    • simulations._get_person_count to simulations.get_person_count
    • Simulation._check_for_cycle to Simulation.check_for_cycle
    • Simulation._check_period_consistency to Simulation.check_period_consistency
    • FlatTracer._enter_calculation to FlatTracer.enter_calculation
    • FlatTracer._exit_calculation to FlatTracer.exit_calculation
    • FlatTracer._start_time to FlatTracer.start_time
    • FlatTracer._end_time to FlatTracer.end_time
    • PerformanceLog._json to PerformanceLog.json
  • Add:
  • periods.year_start
  • periods.year_end
  • Extend:
    • test_runner.YamlItem.repr_failure with a third argument style as per
      the definition of the class it inherits from.

Breaking changes

  • Expire deprecation of:
    • The Dummy class, now provided by commons
    • formula_helpers, now provided by commons
    • memory_config, now provided by experimental
    • rates, now provided by commons
    • Scale, now provided by ParameterScale
    • Bracket, now provided by ParameterScaleBracket
    • ValuesHistory, now provided by ParameterScale
    • ParameterNotFound, now provided by ParameterNotFoundError
    • VariableNameConflict, now provided by VariableNameConflictError
    • VariableNotFound, now provided by VariableNotFoundError
    • simulation_builder, now provided by simulations
    • openfisca-run-test, now provided by openfisca test
    • TaxBenefitSystem.new_scenario, now provided by SimulationBuilder
    • AbstractRateTaxScale, now provided by RateTaxScaleLike
    • AbstractTaxScale, now provided by TaxScaleLike
    • In fine, All errors that were not but are not under errors
  • Delete (no replacement):
    • scripts.measure_performance
    • scripts.migrations.v16_2_to_v17
  • Normalise the use of:
    • file_path for file context-managers (implies argument name rename)

Publising instructions

Because this library has a circular depedency with openfisca-country-template
and openfisca-extension-template, publish of this version requires:

  1. Publishing of one or many release candidates
  2. Publishing next major or openfisca-country-template
  3. Publishing next major or openfisca-extension-template
  4. Publishing next major of this library
  5. Publishing due diligence or unexpected outcome fixes as a release candidates

@bonjourmauko
Copy link
Member Author

I think I went through all of your very helpful suggestions @MattiSG. This PR is in no way exhaustive in terms of style —for example we still. have quote use inconsistency— but it is a very good ground for further improvement.

Copy link
Contributor

@nikhilwoodruff nikhilwoodruff left a comment

Choose a reason for hiding this comment

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

This looks like a nice set of improvements IMO @maukoquiroga.

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 is too big for me to realistically review one more time, especially since the changeset is even bigger now. I have to trust that the initial review followed by pairing is enough 🙂

I believe that, in this case, the usual review approach is unfit for purpose. I see that you intend to release this as 36.0.0.rc1. Indeed, I suggest that we introduce for this case, both as a way to unblock and as an experiment for future similar cases, a prerelease system. Could we publish this as 36.0.0-rc1 (with a dash, not a dot, to abide by SemVer§9 and try it out / ask for it to be tried out on several country packages? 🙂 If no issues arise, then we'll publish as 36.0.0! 😃 If there are issues, then we'll iterate and fix them.

author = 'OpenFisca Team',
author_email = '[email protected]',
name = "OpenFisca-Core",
version = "36.0.0.rc1",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
version = "36.0.0.rc1",
version = "36.0.0-rc1",

@bonjourmauko
Copy link
Member Author

This is too big for me to realistically review one more time, especially since the changeset is even bigger now. I have to trust that the initial review followed by pairing is enough 🙂

I believe that, in this case, the usual review approach is unfit for purpose. I see that you intend to release this as 36.0.0.rc1. Indeed, I suggest that we introduce for this case, both as a way to unblock and as an experiment for future similar cases, a prerelease system. Could we publish this as 36.0.0-rc1 (with a dash, not a dot, to abide by SemVer§9 and try it out / ask for it to be tried out on several country packages? 🙂 If no issues arise, then we'll publish as 36.0.0! 😃 If there are issues, then we'll iterate and fix them.

The easier for that is then to improve the tooling with poetry or hatch to make it as easy as possible to hand publish.

@MattiSG
Copy link
Member

MattiSG commented Nov 16, 2022

I understand changing the dependency manager will make it easier in the future, but I would prefer we avoid putting this on the critical path of unblocking this PR, since this change will certainly yield additional decisions and issues.

@bonjourmauko
Copy link
Member Author

@MattiSG #1160

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 kind:roadmap A group of issues, constituting a delivery roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants