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

Switch to pyproject.toml #408

Merged
merged 4 commits into from
Jun 23, 2024
Merged

Switch to pyproject.toml #408

merged 4 commits into from
Jun 23, 2024

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented May 7, 2024

Alternative to #397: trying to change as little as possible, and will follow with more cleanups if possible

Keep setuptools, remove Poetry.
Remove references to pycodestyle, flake8 already runs it
Make lint configurations consistent with each other
Run black in CI
Remove travis CI
Remove pylint config, it is not enforced (can be added later)

@Viicos
Copy link
Contributor Author

Viicos commented May 7, 2024

@pitbulk would it be possible to run CI on each push and not have to approve the changes? I'll probably have to do a lot of trial and error

@pitbulk
Copy link
Contributor

pitbulk commented Jun 18, 2024

@Viicos, sorry for the delay in taking care of this one.

@deronnax provided the following doc from 2021
where was mentioned that setup.py should not be invoked directly, but in the same doc I read:

This does not mean that setuptools itself is deprecated, or that using setup.py to configure your package builds is going to be removed. The only thing you must stop doing is directly executing the setup.py file — instead delegate that to purpose-built or standards-based tools, preferably those that work with any build backend.

With that in mind, you can review that the CI does not uses any direct call to setup.py (only in the coveralls part that I was investigating).

I had no much idea when I introduced the use of pyproject.toml and poetry, so I checked some big projects and followed in some way what they did.

I appreciate your effort to clean the toolkit and remove unnecessary info and fix any conflict I could introduce during the adoption of poetry and pyproject.

I would prefer to keep setup.py/setup.cfg if possible, as I'm currently doing push on pypi using the setup command, but if you can explain to me the benefits of removing it and start doing the builds and publication in another way, now that we are about to remove python2 support, let me know.

I see other great Python projects having pyproject and setup.py at the same time with no issues:

Let me know what do you think.

@Viicos
Copy link
Contributor Author

Viicos commented Jun 18, 2024

Thanks for coming back at this PR!

I would prefer to keep setup.py/setup.cfg if possible, as I'm currently doing push on pypi using the setup command, but if you can explain to me the benefits of removing it and start doing the builds and publication in another way, now that we are about to remove python2 support, let me know.

Having a setup.py is indeed perfectly fine and isn't deprecated. You can specify the metadata in the setup.py file. The only benefit (apart from being on par with the latest standard) of using pyproject.toml is to have everything specified in a single file: project metadata, tools configuration, etc. You will also benefit from having everything stated explicitly, which is not always the case when using setup.py (in the case of your first Python project example, requirements are defined dynamically, which makes it hard to check for dependencies by simply looking at the GH repo).

Using the setup command to push to PyPI will fail at some point as it is deprecated. Instead, build and twine should be used. Everything is documented here (the official PyPA packaging guide):

@deronnax
Copy link
Contributor

deronnax commented Jun 19, 2024

Plus declarative config is superior to imperative, dynamic config, so does say setuptools:

This approach not only allows automation scenarios but also reduces boilerplate code in some cases.

(I would change in some cases by in all cases)

@pitbulk
Copy link
Contributor

pitbulk commented Jun 20, 2024

@Viicos , do you have a chance to update the PR? otherwise I will.

@Viicos
Copy link
Contributor Author

Viicos commented Jun 20, 2024

@Viicos , do you have a chance to update the PR? otherwise I will.

I'll be able to get back to it shortly

@pitbulk
Copy link
Contributor

pitbulk commented Jun 20, 2024

@Viicos, It seems it fails for python 3.6 due your requirement in pyproject.toml

requires = ["setuptools>=61.0.0"]

Do you see any problem requiring a lower version?

And you need to whitelist complexity on lint:

src/onelogin/saml2/settings.py:413:5: C901 'OneLogin_Saml2_Settings.check_sp_settings' is too complex (31)
src/onelogin/saml2/response.py:49:5: C901 'OneLogin_Saml2_Response.is_valid' is too complex (48)

So let's set at .flake8

max-complexity = 50

@Viicos
Copy link
Contributor Author

Viicos commented Jun 20, 2024

Yep I'll take a look, iirc downgrading the setuptools version should work

@Viicos
Copy link
Contributor Author

Viicos commented Jun 21, 2024

I take that back, setuptools==61.0.0 is the first version to support pyproject.toml, but requires Python >= 3.7. Any chance we could drop support for (at least) Python 3.6? It has been EOL for nearly 4 years (so is 3.7, and 3.8 soon).

@pitbulk
Copy link
Contributor

pitbulk commented Jun 21, 2024

I think it's ok to drop 3.6 support then.

In case of any security issue, I could drop a 1.16.1 version addressing this to cover people using Python < 3.6

Send a commit removing 3.6, and adding 3.12 from CI

Keep setuptools, remove Poetry.
Remove references to pycodestyle, flake8 already runs it
Make lint configurations consistent with each other
Run black in CI
Remove travis CI
Remove pylint config, it is not enforced (can be added later)
@Viicos
Copy link
Contributor Author

Viicos commented Jun 21, 2024

@pitbulk, what do you want to do with the coveralls task? Should we remove it?

Also, source code will need to be formatted, as black doesn't seem to have been applied for a while.

@pitbulk pitbulk merged commit 7658b88 into SAML-Toolkits:master Jun 23, 2024
6 of 7 checks passed
@Viicos Viicos deleted the pyproject branch June 23, 2024 20:13
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.

3 participants