-
Notifications
You must be signed in to change notification settings - Fork 14
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
build: update openfisca-core to 42.0.0 #302
Conversation
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.
Thanks Mauko, just a warning about using url in dependency, but maybe it was just for testing ?
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.
Please merge only after openfisca-core have been tested against france tests
@benjello @benoit-cty As with France, this PR can't be merged as is. The other PRs need to be merged first, then I'll update to the correct PyPi package here. |
58bc016
to
056a08a
Compare
@benjello @benoit-cty I let you merge this one? As I'm not a maintainer of this repo. |
@benoit-cty @clallemand @sylvainipp @Lolajossipp: I would merge this PR. Unless you are in the middle of delicate work and it could be harmful to you. I can also ask for a major version change. But keeping a minor change would allow bugs to appear that would force us to test this package better to avoid this kind of problems next time. Thanks for your feedback! |
I'm OK to merge. The CI tests can't pass because of the wrong import of Core, so the sooner the better from my point of view. |
@benoit-cty can you approve the changes now ? |
@benjello Can I test this PR in our repos today before you merge it ? Just to be shure its a patch bump version |
Sure @clallemand |
@benjello @benoit-cty I've changed my mind. I can't test it because of openfisca-france, but you can merge it not to block @sylvainipp 's work on this repo |
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.
I think there is a typo with a double indentation. Beside that I'm OK. Thanks @bonjourmauko !
Co-authored-by: Benoit Courty <[email protected]>
Depends on openfisca/openfisca-core#1223
Depends on openfisca/country-template#154
These changes: