SAML-Toolkits / python3-saml

MIT License
670 stars 302 forks source link

Switch to `pyproject.toml` #408

Closed Viicos closed 2 weeks ago

Viicos commented 2 months ago

Alternative to https://github.com/SAML-Toolkits/python3-saml/pull/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 commented 2 months ago

@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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

Viicos commented 2 weeks ago

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

I'll be able to get back to it shortly

pitbulk commented 2 weeks ago

@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 commented 2 weeks ago

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

Viicos commented 2 weeks ago

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 commented 2 weeks ago

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

Viicos commented 2 weeks ago

@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.