Skip to content
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

Add version 17 #226

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Add version 17 #226

merged 1 commit into from
Nov 7, 2023

Conversation

etobella
Copy link
Member

@etobella etobella commented Nov 6, 2023

Depends on OCA/oca-ci#54

Comment on lines +35 to +37
- odoo-version: 17.0
python-version: "3.10"
machine: ubuntu-22.04
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- odoo-version: 17.0
python-version: "3.10"
machine: ubuntu-22.04
- odoo-version: 17.0
python-version: "3.11"
machine: ubuntu-22.04

Does it makes sense to use 3.11 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the test of the template itself. It does not matter that much, I think. For for testing the addons we want to test on 3.10 which is the minimum Python version we want to support (in alignment with what Odoo decided in odoo/odoo#136904).

@pedrobaeza
Copy link
Member

pedrobaeza commented Nov 6, 2023

Is ruff finally going to be adopted?

About the pyproject.toml file, I'm worried about the contamination it provokes on the same module folder.

@sbidoul
Copy link
Member

sbidoul commented Nov 7, 2023

Is ruff finally going to be adopted?

I've not got much feedback. I use it on a few projects and it works really well. I propose to enable it, and we can still revert if it proves problematic.

About the pyproject.toml file, I'm worried about the contamination it provokes on the same module folder.

This is indeed a new file in each addon. In exchange we remove all setup/*/setup.py and related symbolic links.
These are very small files that will be ignored by Odoo.

And they open the possibility to declare more advanced dependencies, such as saying an addon requires version 'account_payment_order>=16.0.2.0.0' or something similar. We had that capability in setup.py before but it was not used because not known, but also harder to discover with those setup.py files hidden away in another folder than the addon.

With pyproject.toml in the addon directory, we have everything related to an addon in the same place. It is much cleaner in the end.

@pedrobaeza
Copy link
Member

Let's enable ruff then.

About pyproject.toml, all the files are equal unless you need a special setup. Please don't add these files and create them "on the fly" for the build you do in whool or where it takes place, and only force to create it when something special is needed. That's the cleanest way and the way to avoid that other setups get contaminated with non needed files.

@sbidoul
Copy link
Member

sbidoul commented Nov 7, 2023

Pedro, this has been done like this for setup.py for 8 years. This is just a different file.
It is necessary not only for the build, but also to include test requirements from git (for unmerged PRs).

@sbidoul
Copy link
Member

sbidoul commented Nov 7, 2023

@etobella the test for 17 is red, likely because we should check for the presence of .ruff.toml instead of .flake8.

@pedrobaeza
Copy link
Member

That doesn't mean that I like that (and the same for all the non-PIP people), but at least, they were in other place, not on the same module folder. As said, all of them being the default values and a very simple file, wherever they are needed, they can be created on the fly by the tooling. Don't you think it's not too much to ask?

It's also an insurance that if the format changes by whatever reason, you don't have to recreate all of them in other manner and provoke a lot of diff and noise.

@sbidoul
Copy link
Member

sbidoul commented Nov 7, 2023

As said, all of them being the default values and a very simple file, wherever they are needed, they can be created on the fly by the tooling

That is technically not possible for the CI.

@pedrobaeza
Copy link
Member

Well, you know that everything is possible with some effort. I don't like that a deployment preference conditions the OCA structure.

@sbidoul
Copy link
Member

sbidoul commented Nov 7, 2023

This is not a deployment preference. All the dependency management of our CI is based on this.

And talking about effort, I maintain most of this stuff for free, and I can tell you that this approach requires much less effort to keep the system robust than what we had before. There is no way we are going to step back. This pyproject.toml is just a modernization of a system that has been working well for many years now. And after a few weeks you will not even notice these files anymore :)

@pedrobaeza
Copy link
Member

Well, what do you want me to say? Of course I'm very thankful by all your work in the OCA tooling, but at the end, it's very conditioned by your deployment preference with PIP. Let's continue, as like you said, this can't be stepped back, and we are far better than where we were some years ago.

@legalsylvain
Copy link
Contributor

Is it possible to make PR green even if the pytoml file is absent ?
I mean, that's annoying to force contributors to run locally precommit to generate files that are useless for him.
I think we should ignore the file pyproject.toml in the diff check here,
https://github.com/dixmit/oca-addons-repo-template/blob/master2/src/.github/workflows/pre-commit.yml.jinja#L54
(same as readme.rst file that is ignored, because it will be autogenerated, once the PR is merged)

@sbidoul
Copy link
Member

sbidoul commented Nov 7, 2023

Is it possible to make PR green even if the pytoml file is absent ?

The PR yes, but then that PR would not be usable in test-requirements.txt of other PRs. That is why generating these files is in pre-commit.

What we should reinvestigate is if it is possible for pre-commit to auto-fix PRs. That would be useful for that but also any linter that can autofix. I know https://pre-commit.ci can do it so it might be applicable for us too. Not sure if it is possible with regards to permissions.

@sbidoul sbidoul merged commit 8f0b937 into OCA:master Nov 7, 2023
8 checks passed
@legalsylvain
Copy link
Contributor

The PR yes, but then that PR would not be usable in test-requirements.txt of other PRs. That is why generating these files is in pre-commit.

that's OK for me. I just wish the PR wasn't red, for whatever "pointless reason", so people don't get discouraged.

@sbidoul
Copy link
Member

sbidoul commented Nov 7, 2023

that's OK for me. I just wish the PR wasn't red, for whatever "pointless reason", so people don't get discouraged.

Yes I totally understand that point. Even reformatting by black can be considered pointless and be a hurdle for new contributors. I'll try to reinvestigate the pre-commit autofix approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants