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

[bonjourmauko]

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

[benjello]

Hi @maukoquiroga : I am not very qualified to review all this work which BTW seems very substantial and valuable.

I am just wondering why you changed may methods _foo to foo or even foo_ ?

[bonjourmauko]

Hi @maukoquiroga : I am not very qualified to review all this work which BTW seems very substantial and valuable.

I'd like to have your opinion indeed, the idea would be to remove as many elements of style and keep those that are more tied to syntax and less controversial.

I am just wondering why you changed may methods _foo to foo or even foo_ ?

That's a convention thing, he's the rationale, and why it may or not make sense for us:

Normally when you have _foo it means that a method is for internal use, and not to be called on externally. That's why usually documentation builders ignore them by default, and linters are going to warn you if you call them outside the scope.

So what I did was not to rename them, but to open them: because they're already being used as public methods, I made them public, while keeping the _ for backwards-compatibility.

Where it may not make sense for us is that we reorganised modules in a very modular way, and then we could for example argue that all methods marked as _foo inside the parameters module can be used inside, but not outside, etc.

Regarding foo_: that is just because _set, to be public, becomes set, but set is a builtin function, so it becomes set_.

That's an example of a pure style thing we could choose to ignore, as we do for variables for example, see https://www.python.org/dev/peps/pep-0008/#descriptive-naming-styles.

[bonjourmauko]

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.

[bonjourmauko]

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]

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]

@MattiSG #1160