-
Notifications
You must be signed in to change notification settings - Fork 75
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
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
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.
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 = '[email protected]', | ||
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.cfg
this library does not run with py36, py310, numpy v1.17, etc., which is not
product of any particular change introduced by this changeset)
@staticmethod
whenself
is unusedexcept from
for better exception contextisinstance()
as per the best practice...
as a placeholder andpass
for loopsnp
becomesnumpy
)exec
because dangerous and hard to maintaincommons.imports
make format-style
make test
andmake style-check
New 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_array
toHolder.to_array
Holder._set
toHolder.put
Holder._memory_storage
toHolder.memory_storage
parameters._compose_name
toparameters.compose_name
parameters._validate_parameter
toparameters.validate_parameter
ParameterNodeAtInstant._name
toParameterNodeAtInstant.name
ParameterNodeAtInstant._instant_str
toParameterNodeAtInstant.instant_str
ParameterNodeAtInstant._children
toParameterNodeAtInstant.children
Population._holders
toPopulation.holders
simulations._get_person_count
tosimulations.get_person_count
Simulation._check_for_cycle
toSimulation.check_for_cycle
Simulation._check_period_consistency
toSimulation.check_period_consistency
FlatTracer._enter_calculation
toFlatTracer.enter_calculation
FlatTracer._exit_calculation
toFlatTracer.exit_calculation
FlatTracer._start_time
toFlatTracer.start_time
FlatTracer._end_time
toFlatTracer.end_time
PerformanceLog._json
toPerformanceLog.json
periods.year_start
periods.year_end
test_runner.YamlItem.repr_failure
with a third argumentstyle
as perthe definition of the class it inherits from.
Breaking changes
Dummy
class, now provided bycommons
formula_helpers
, now provided bycommons
memory_config
, now provided byexperimental
rates
, now provided bycommons
Scale
, now provided byParameterScale
Bracket
, now provided byParameterScaleBracket
ValuesHistory
, now provided byParameterScale
ParameterNotFound
, now provided byParameterNotFoundError
VariableNameConflict
, now provided byVariableNameConflictError
VariableNotFound
, now provided byVariableNotFoundError
simulation_builder
, now provided bysimulations
openfisca-run-test
, now provided byopenfisca test
TaxBenefitSystem.new_scenario
, now provided bySimulationBuilder
AbstractRateTaxScale
, now provided byRateTaxScaleLike
AbstractTaxScale
, now provided byTaxScaleLike
errors
scripts.measure_performance
scripts.migrations.v16_2_to_v17
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:openfisca-country-template
openfisca-extension-template