-
Notifications
You must be signed in to change notification settings - Fork 83
Unify syntax, add style checks, remove obsolete Python 2 leftovers #1047
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
base: master
Are you sure you want to change the base?
Conversation
|
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. |
d70b861 to
5c1c382
Compare
nikhilwoodruff
left a comment
There was a problem hiding this 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.
MattiSG
left a comment
There was a problem hiding this 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 = 'contact@openfisca.org', | ||
| name = "OpenFisca-Core", | ||
| version = "36.0.0.rc1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| version = "36.0.0.rc1", | |
| version = "36.0.0-rc1", |
The easier for that is then to improve the tooling with |
|
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. |
Superset of openfisca/country-template#97
Follows discussion on #1033
Supersedes #977
Supersedes #960
Depends on #1160
Technical changes
setup.cfgthis library does not run with py36, py310, numpy v1.17, etc., which is not
product of any particular change introduced by this changeset)
@staticmethodwhenselfis unusedexcept fromfor better exception contextisinstance()as per the best practice...as a placeholder andpassfor loopsnpbecomesnumpy)execbecause dangerous and hard to maintaincommons.importsmake format-stylemake testandmake style-checkNew features
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_arraytoHolder.to_arrayHolder._settoHolder.putHolder._memory_storagetoHolder.memory_storageparameters._compose_nametoparameters.compose_nameparameters._validate_parametertoparameters.validate_parameterParameterNodeAtInstant._nametoParameterNodeAtInstant.nameParameterNodeAtInstant._instant_strtoParameterNodeAtInstant.instant_strParameterNodeAtInstant._childrentoParameterNodeAtInstant.childrenPopulation._holderstoPopulation.holderssimulations._get_person_counttosimulations.get_person_countSimulation._check_for_cycletoSimulation.check_for_cycleSimulation._check_period_consistencytoSimulation.check_period_consistencyFlatTracer._enter_calculationtoFlatTracer.enter_calculationFlatTracer._exit_calculationtoFlatTracer.exit_calculationFlatTracer._start_timetoFlatTracer.start_timeFlatTracer._end_timetoFlatTracer.end_timePerformanceLog._jsontoPerformanceLog.jsonperiods.year_startperiods.year_endtest_runner.YamlItem.repr_failurewith a third argumentstyleas perthe definition of the class it inherits from.
Breaking changes
Dummyclass, now provided bycommonsformula_helpers, now provided bycommonsmemory_config, now provided byexperimentalrates, now provided bycommonsScale, now provided byParameterScaleBracket, now provided byParameterScaleBracketValuesHistory, now provided byParameterScaleParameterNotFound, now provided byParameterNotFoundErrorVariableNameConflict, now provided byVariableNameConflictErrorVariableNotFound, now provided byVariableNotFoundErrorsimulation_builder, now provided bysimulationsopenfisca-run-test, now provided byopenfisca testTaxBenefitSystem.new_scenario, now provided bySimulationBuilderAbstractRateTaxScale, now provided byRateTaxScaleLikeAbstractTaxScale, now provided byTaxScaleLikeerrorsscripts.measure_performancescripts.migrations.v16_2_to_v17file_pathfor file context-managers (implies argument name rename)Publising instructions
Because this library has a circular depedency with
openfisca-country-templateand
openfisca-extension-template, publish of this version requires:openfisca-country-templateopenfisca-extension-template