-
Notifications
You must be signed in to change notification settings - Fork 3
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: test built version #54
Conversation
4094ef1
to
d40bce1
Compare
Could you take a look now? |
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.
Not really qualified for this PR.
I just noticed something weird.
You may need @benoit-cty or @sandcha reviewing it.
Might also be nice if changes made here are propagated to france (or vice versa) to keep the tooling in line.
openfisca_extension_template/parameters/local_town/child_allowance/index.yaml
Outdated
Show resolved
Hide resolved
openfisca_extension_template/parameters/local_town/child_allowance/amount.yaml
Outdated
Show resolved
Hide resolved
What is weird?
I'm trying (since 2021) to equalise tooling of core/country/extension. Happy to help with France, it comes right after in the pipe. |
The yamlint addition |
I will make my point clearer : @benoit-cty and @sandcha worked a lot on the tooling in france (and I don't know if ti was portted to country-template and extensions) so I just warned about divergences and double work. |
Ah. I did add the yaml check as it is run in country-template. Maybe do we have a yamllint config in core or france? |
In OpenFisca-France, we did have this config in
But I can't say much about it, I never work on it. |
My comment where from 2 years ago. Why ? That being said, we are using Poetry in all our Python project at LexImpact so it's easier to get support than on Hatch or UV... |
Sorry I didn't properly reply in my earlier comment. I'll do below:
There has been scattered discussions mainly in core. The last general discussion on roadmap is this one: openfisca/openfisca-core#942 The last council decision (RFC) on tooling is this one: openfisca/openfisca-core#1040 (comment) And the general problem this PR aims to fix is about properly testing against built versions openfisca/openfisca-core#1064
In particular, this PR relies on the virtuous interaction of I could still achieve the same result, although more laboriously —which we want to avoid, as these things have to be maintained over time— without So, it that is a blocker, I can remove it (as, again, at least here, is only accessory).
I have not knowledge on any of these tools. However, I'm personally agnostic on the tooling given that:
If we adopt a tool replacing make (which is ok, as seen in the RFC), the big feature it has to have is to allow for easy parallelisation of tasks. As
And that is a good point. So long as any tool uses a more or less standard pyproject.toml, adopting one tool or the other, at least for the use case of this PR, is an easily revertable or revokable decision. All that being said, I'd happily take a look at Thoughts? |
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.
If I install with make install
, then do make format
I end up with:
/bin/sh: 1: black: not found
I think it's a blocker but maybe I'm missing a command ?
I suggest to put poetry run
in front of all command : dad766d
Why don't use poetry env remove --all
in make uninstall
instead of pip
?
Thanks for the details ! As you say, using Poetry is a nice step. It doesn't prevent using another tool in the future. It's not blocking at all 👍 If you are curious about Hatch, I use it for https://github.com/mlco2/codecarbon/blob/master/CONTRIBUTING.md#some-hatch-commands where it replace venv, poetry, make and tox. But it can be a huge step for the OpenFisca community. |
I'll try that. |
Ping @benoit-cty |
And due to some overly-clever ways that core handle things, I always fear that big changes will break things.
Thanks! That being said, the builder protocol is now standard, so we can perfectly mix together poetry and hatch. In another project, where I have to compile some extensions in C and C++, I use poetry + mason, for example. |
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.
LGTM !
Part of openfisca/openfisca-core#1064
Depends on openfisca/country-template#159
poetry
to ensure deterministic buildstox
to test builds in isolation