Pyproject support #34

gpongelli · 2023-01-24T10:49:57Z

Thanks for this awesome package, I need it for my Poetry-managed projects ;) !

This is a possible support for pyproject.toml file managed with Poetry, many methods are reused from setup.py source.

Extra package to be installed is pytomlpp to manage toml files.

closes #21 .

EDIT: added management for setuptools and flit pyproject files

mgedmin

Thank you very much! I like most of the changes.

The Windows build failures are a bit of a blocker at the moment. And I suspect some of the unit tests are unnecessary, as they look like they were adapted from setup.py support routines that try to handle dynamic code instead of declarative values. Does poetry really support Python list comprehensions inside the pyproject.toml file?

mgedmin · 2023-01-24T13:07:06Z

setup.py

@@ -68,6 +68,6 @@
            'check-python-versions = check_python_versions.cli:main',
        ],
    },
-    install_requires=['pyyaml'],
+    install_requires=['pyyaml', 'pytomlpp'],


I'd rather use tomli; python_version < 3.11 (except with the correct syntax for the python version check).

EDIT: unless pytomlpp is required for formatting-preserving write support or something like that.

Why are Appveyor builds failing to build pytomlpp? I see Windows wheels on PyPI, why is pip install not using them?

It's not clear for me why pytomlpp is building on windows even if wheel exists, I've never used appveyor so I've no idea of what's happening.

I've no problems to change TOML package, even if I saw pytomlpp is faster, I use it just for this reason.

tomlkit seems to be the only library that preserves comments.

tomli has an explicit FAQ answer about comments.

mgedmin · 2023-01-24T13:08:32Z

pyproject.toml

@@ -0,0 +1,38 @@
+[tool.poetry]
+    name = "check-python-versions"
+    version = "1.0.0"


I don't want to manually keep two sets of metadata in sync. All my current maintenance tooling supports reading/updating the version number in src/check_python_versions/__init__.py.

I've never used Poetry and I'm not sure I want to switch to it at this time.

If you need it for the test suite, I suggest moving it inside tests/ somewhere.

I put version metadata there to mimic the setup.py content, not to be used to build the package.

Into my packages, I usually have version number into __init__.py and into pyproject.toml, kept in sync with commitizen tool, FYI .

let me know if you prefer removing the version here.

Version removed.

mgedmin · 2023-01-24T13:11:28Z

src/check_python_versions/sources/poetry_pyproject.py

@@ -0,0 +1,197 @@
+"""
+Support for pyproject.toml.


But AFAIU Poetry is not the only build tool that supports pyproject.toml? There are also things like flit, and I think even setuptools? (I'm not sure about setuptools -- I know there's a way to use declarative configuration for setuptools with an empty/minimal setup.py, but I don't know if it goes into setup.cfg, or if it can also go into pyproject.toml.)

I think it can be made to work fine if you make the extract() callback of the new Source check the value of [build-system] build-backend and return None if it doesn't match poetry.whatever. Then multiple sources can declare that they support pyproject.toml and they should all cooperate nicely.

But AFAIU Poetry is not the only build tool that supports pyproject.toml? There are also things like flit, and I think even setuptools? (I'm not sure about setuptools -- I know there's a way to use declarative configuration for setuptools with an empty/minimal setup.py, but I don't know if it goes into setup.cfg, or if it can also go into pyproject.toml.)

yes, recently many tools are starting support pyproject.toml instead of setup.py, but the content could differ (see setuptools as example, it does not start with [tool.poetry] section but with [project] )

I think it can be made to work fine if you make the extract() callback of the new Source check the value of [build-system] build-backend and return None if it doesn't match poetry.whatever. Then multiple sources can declare that they support pyproject.toml and they should all cooperate nicely.

surely it can be improved with other tools that are using pyproject.toml, I've to "study" your suggestion 😄

I think it can be made to work fine if you make the extract() callback of the new Source check the value of [build-system] build-backend and return None if it doesn't match poetry.whatever. Then multiple sources can declare that they support pyproject.toml and they should all cooperate nicely.

Actually it's done, there are a plenty of utility methods to check which is the tools that handles the pyproject.toml file, with their tests.

some part are left unimplemented (even if the call is commented, e.g. this point and this and this) because I don't know the pyproject.toml structure used by setuptools and flit.

mgedmin · 2023-01-24T13:12:04Z

src/check_python_versions/sources/poetry_pyproject.py

+
+
+def _get_pyproject_toml_classifiers(
+        filename: FileOrFilename = PYPROJECT_TOML


This is a bit over-indented.

mgedmin · 2023-01-24T13:12:10Z

src/check_python_versions/sources/poetry_pyproject.py

+
+
+def _get_pyproject_toml_python_requires(
+        filename: FileOrFilename = PYPROJECT_TOML


mgedmin · 2023-01-24T13:15:09Z

src/check_python_versions/sources/poetry_pyproject.py

+
+    table['tool']['poetry']['classifiers'] = new_value
+
+    return pytomlpp.dumps(table).split('\n')


Ah! Does this preserve formatting/comments? This would be a sufficiently good reason not to use tomli.

I've done a test with my pyproject, and the dumped content is different than original one.
I've searched pytomlpp and seems there's no way to keep formatting/comments (and also the original toml style).

that dumps is used to create a big string with the content of entire file.

Switched to tomlkit that keeps the original style and comments.

mgedmin · 2023-01-24T13:15:30Z

src/check_python_versions/sources/poetry_pyproject.py

+
+def _update_pyproject_toml_python_requires(
+        filename: FileOrFilename,
+        new_value: Union[str, List[str]]


This is overindented, and also I would like a trailing comma here.

mgedmin · 2023-01-24T13:17:52Z

tests/sources/test_poetry_pyproject.py

+            [tool.poetry.dependencies]
+                python = ', '.join([
+                '>=2.7',
+                '!=3.0.*',


Is this actually valid TOML syntax, supported by Poetry?

it's not a valid syntax

mgedmin · 2023-01-24T13:21:04Z

tests/sources/test_poetry_pyproject.py

+            name='foo',
+            classifiers=[
+                'Programming Language :: Python :: %s' % v
+                for v in ['2.7', '3.7']


Is this actually valid syntax?

Is this actually valid syntax?

no, I'm going to remove the tests that uses this declaration way

mgedmin · 2023-01-24T13:21:46Z

tests/sources/test_poetry_pyproject.py

+    assert (
+        "Did not understand python_requires formatting in python dependency"
+        in capsys.readouterr().err
+    )


I would like to see a unit test that modifying the toml file leaves comments in place.

added dedicated test

setuptools and flit are not implemented because struct not known

gpongelli · 2023-01-24T18:15:26Z

Thank you very much! I like most of the changes.

The Windows build failures are a bit of a blocker at the moment. And I suspect some of the unit tests are unnecessary, as they look like they were adapted from setup.py support routines that try to handle dynamic code instead of declarative values. Does poetry really support Python list comprehensions inside the pyproject.toml file?

Removed the tests with dynamic code. Actually there is a minimum set of working tests and the build pass because of different toml library.

gpongelli · 2023-01-25T09:42:27Z

added implementation&tests for setuptools and flit tools

mgedmin

No time to do a full review today, but here are a few comments

mgedmin · 2023-01-25T13:42:25Z

pyproject.toml

@@ -1,6 +1,5 @@
 [tool.poetry]
    name = "check-python-versions"
-    version = "1.0.0"


It's not the version that's a problem, it's that this project (check-python-versions) doesn't use Poetry and therefore it shouldn't have Poetry configuration in its top-level pyproject.toml.

Please move this file into tests/sources/data/ or a similarly named subdirectory if it's needed by the test suite.

it's clear that this project doesn't use poetry to build it.

I've made the file to do a live check with check-python-versions.py , so it could be related to test_cli.py , I've to look at it.

mgedmin · 2023-01-25T13:45:08Z

src/check_python_versions/sources/poetry_pyproject.py

@@ -38,11 +39,29 @@
 PYPROJECT_TOML = 'pyproject.toml'


+def _load_toml(filename: FileOrFilename):


Would be nice if there was a type annotation for the return type. I suppose it's hard to be more specific than a generic Dict[str, Any].

seems this row is outdated, also the file was renamed to "pyproject.py" because it handles all the tools that use pyproject.toml

mgedmin · 2023-01-25T13:45:39Z

src/check_python_versions/sources/poetry_pyproject.py

+        with open_file(filename) as fp:
+            table = load(fp)
+    if isinstance(filename, TextIO):
+        table = load(filename)


There's a check_python_version.utils.open_file() context manager that could simplify this.

mgedmin · 2023-01-25T13:46:56Z

src/check_python_versions/sources/poetry_pyproject.py

@@ -11,7 +11,8 @@
 check-python-versions supports both.
 """

-import pytomlpp
+from tomlkit import dumps
+from tomlkit import parse, load


flake8 tells me tomlkit.parse is imported but unused.

mgedmin · 2023-01-25T13:48:38Z

src/check_python_versions/sources/poetry_pyproject.py

+def get_toml_content(
+    filename: FileOrFilename = PYPROJECT_TOML
+) -> FileLines:
+    """Utility method to see if TOML library keeps style and comments."""


If it's only used by the test suite, it should be moved into test_pyproject.py.

mgedmin · 2023-01-25T13:52:44Z

src/check_python_versions/sources/poetry_pyproject.py

@@ -94,7 +116,7 @@ def get_python_requires(
    if python_requires is None:
        return None
    if not isinstance(python_requires, str):
-        warn('The value passed to python is not a string')
+        warn('The value passed to python dependency is not a string')


mgedmin · 2023-01-25T13:55:26Z

src/check_python_versions/sources/poetry_pyproject.py

@@ -111,7 +133,7 @@ def update_supported_python_versions(
    if classifiers is None:
        return None
    if not isinstance(classifiers, (list, tuple)):
-        warn('The value passed to setup(classifiers=...) is not a list')
+        warn('The value passed to classifiers is not a list')


mgedmin · 2023-01-25T13:56:57Z

src/check_python_versions/sources/poetry_pyproject.py

@@ -175,7 +195,7 @@ def _update_pyproject_toml_python_requires(
        return []

    table['tool']['poetry']['dependencies']['python'] = new_value
-    return pytomlpp.dumps(table).split('\n')
+    return dumps(table).split('\n')


Note to self: investigate what tomlkit.dumps() returns and whether this should be using .splitlines() instead.

Note to self: make sure the unit tests verify that modifying a .toml file doesn't add or remove any trailing newlines at the end of the FILE.

mgedmin · 2023-01-25T13:58:08Z

tests/sources/test_poetry_pyproject.py

+                                               '        \'Programming Language :: Python :: 3.6\',',
+                                               '        \'Programming Language :: Python :: 3.10\',',
+                                               '    ]',
+                                               '']


This would read better without the excessive indentation. Look at how other unit tests make assertions about the output. Here is one example:

check-python-versions/tests/sources/test_github.py

Lines 229 to 245 in 82c67ea

assert "".join(result) == textwrap.dedent("""\

jobs:

build:

strategy:

matrix:

config:

# [Python version, tox env]

- ["3.8", "lint"]

- ["2.7", "py27"]

- ["3.8", "py38"]

- ["3.9", "py39"]

- ["pypy2", "pypy"]

- ["pypy3", "pypy3"]

- ["3.8", "coverage"]

steps:

- ...

""")

mgedmin · 2023-01-25T13:59:07Z

tests/sources/test_poetry_pyproject.py

+#     assert (
+#         'The value passed to classifiers is not a list'
+#         in capsys.readouterr().err
+#     )


Please do not leave commented-out tests in the file. Either remove entirely, or fix to make them pass, if it's a useful test.

it's an outdated state, latest commit have changed the file name to test_pyproject_poetry.py

mgedmin · 2023-01-26T18:17:09Z

Do tell me when you want me to take another look. I see there are still test failures, so I was thinking of waiting a bit.

gpongelli · 2023-01-26T18:59:18Z

Do tell me when you want me to take another look. I see there are still test failures, so I was thinking of waiting a bit.

They’re failing because coverage is less than 100% .

An improvement: is it possible to run both appveyor and GitHub action on all pushes? Because fixing issues from GitHub actions (that does not run always) after had fixed appveyor ones is a little bit demoralising for whom would contribute.

mgedmin · 2023-01-26T19:06:40Z

Looks like GitHub requires me to approve the action run every time ("for first-time contributors"). I don't know if there's a way to avoid that.

Oh how I hate cryptocurrency scammers that started abusing free CI and caused every CI provider to adopt these countermeasures.

gpongelli · 2023-01-27T12:50:41Z

Looks like GitHub requires me to approve the action run every time ("for first-time contributors"). I don't know if there's a way to avoid that.

Is it something you can configure into settings?

Oh how I hate cryptocurrency scammers that started abusing free CI and caused every CI provider to adopt these countermeasures.

Didn’t know about that.

anyway, just reached again 100% of coverage.

gpongelli · 2023-01-28T13:32:32Z

Do tell me when you want me to take another look. I see there are still test failures, so I was thinking of waiting a bit.

@mgedmin you can do it now, I've run all the checks locally and they all pass.

mgedmin

Sorry for the delay, I was mostly offline for personal reasons.

I found a setting that relaxes the approval requirement for pull request to first-time GitHub users. Thank you for suggesting that there ought to be a setting for that somewhere!

There are some style concerns I have, but they're not critical. I might as well just merge this and fix things up more to my liking in git master.

Thank you!

The most important missing bit is describing the new feature in the changelog and crediting you.

src/check_python_versions/sources/pyproject.py

mgedmin · 2023-02-07T08:20:42Z

src/check_python_versions/sources/pyproject.py

+    if PYTHON not in \
+            table[TOOL][POETRY][DEPENDENCIES]:
+        return None
+    return cast(str, table[TOOL][POETRY][DEPENDENCIES][PYTHON])


Um, I checked Poetry's documentation and tools.poetry.dependencies.python uses a different syntax (^3.8 instead of >= 3.8), so this will not work right.

mgedmin · 2023-02-07T08:48:46Z

tests/sources/test_pyproject_setuptools.py

+    """))
+    fp.name = "pyproject.toml"
+    result = update_python_requires(fp, v(['2.7', '3.2']))
+    assert result == ['[project]',


This is wrong: the FileLines returned by update_... functions are supposed to include trailing newlines.

mgedmin · 2023-02-07T12:14:13Z

mypy 1.0 was released today (or yesterday? timezones, eh) and started complaining a lot about pyproject.py. While trying to fix those errors I ended up rewriting most of the code. I fixed some bugs that I noticed, but I'm afraid I might have introduced new ones, so some testing with actual Poetry-using projects would be appreciated.

mgedmin · 2023-02-07T12:15:43Z

(None of the above would've happened if you hadn't opened this PR, for which I would once again like to thank you!)

gpongelli added 7 commits January 22, 2023 18:41

pyproject.toml poetry file

b2e74b5

search for pyproject.toml file

7d36495

add library to parse pyproject and add new file to SOURCES

45540d7

implement pieces for pyproject.toml handling

fd5c470

return FileLines instead of dict

51c045f

first version for tests

f54b957

build-system seems interfere with appveyor

d8bc619

mgedmin requested changes Jan 24, 2023

View reviewed changes

gpongelli added 10 commits January 24, 2023 15:09

fix deprecation warning

a5a6565

indentation

b15d091

fix message when no required files are present

08a92d5

toml does not need comma

16f0c7d

removed version from toml

76c40f2

using tomlkit package

483d27b

moved to tomlkit, small set of working tests

0aca989

manage different tools that use pyproject.toml

ac1b95d

setuptools and flit are not implemented because struct not known

renamed module to generic pyproject.py

dad4492

renamed objects removing poetry reference

8a515de

gpongelli added 9 commits January 25, 2023 09:48

tool.setuptools is optional table

7ea9050

removed wrong test

12c5265

same test name as setup.py

ed48eda

unsupported dynamic format on toml file

d748483

manage StringIO input

583c366

fixed latest tests

b10da17

setuptools pyproject test and implementation

8349504

renamed files to have all pyproject tests with same prefix

c7e1623

added flit implementation and tests

82c67ea

gpongelli changed the title ~~Poetry Pyproject support~~ Pyproject support Jan 25, 2023

gpongelli requested a review from mgedmin January 25, 2023 09:56

mgedmin reviewed Jan 25, 2023

View reviewed changes

gpongelli added 9 commits January 25, 2023 15:50

fix trailing whitespace

63e735a

review comments

7ed0b9a

pass flake8 tests

605fe9a

new tests for cli

c166aed

fix after latest changes

f2c7d21

fix test

2a0d7d4

fix isort stage

1214b38

install tomlkit for mypy run

572ca6e

fix mypy checks

7e471e4

gpongelli added 4 commits January 27, 2023 12:40

removed unused pyproject.toml file

4539a49

checks done in previous call on the chain

f1cb32e

new tests and changes to reach 100%

de13d7a

test file names

04c8988

mgedmin approved these changes Feb 2, 2023

View reviewed changes

src/check_python_versions/sources/pyproject.py Show resolved Hide resolved

mgedmin merged commit 925b32c into mgedmin:master Feb 2, 2023

mgedmin reviewed Feb 7, 2023

View reviewed changes



		def _get_pyproject_toml_classifiers(
		filename: FileOrFilename = PYPROJECT_TOML



		def _get_pyproject_toml_python_requires(
		filename: FileOrFilename = PYPROJECT_TOML


		table['tool']['poetry']['classifiers'] = new_value

		return pytomlpp.dumps(table).split('\n')

		@@ -38,11 +39,29 @@
		PYPROJECT_TOML = 'pyproject.toml'


		def _load_toml(filename: FileOrFilename):

	assert "".join(result) == textwrap.dedent("""\
	jobs:
	build:
	strategy:
	matrix:
	config:
	# [Python version, tox env]
	- ["3.8", "lint"]
	- ["2.7", "py27"]
	- ["3.8", "py38"]
	- ["3.9", "py39"]
	- ["pypy2", "pypy"]
	- ["pypy3", "pypy3"]
	- ["3.8", "coverage"]
	steps:
	- ...
	""")

Pyproject support #34

Pyproject support #34

Conversation

gpongelli commented Jan 24, 2023 • edited Loading

mgedmin left a comment

Choose a reason for hiding this comment

mgedmin Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpongelli Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpongelli commented Jan 24, 2023

gpongelli commented Jan 25, 2023

mgedmin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgedmin commented Jan 26, 2023

gpongelli commented Jan 26, 2023

mgedmin commented Jan 26, 2023

gpongelli commented Jan 27, 2023

gpongelli commented Jan 28, 2023

mgedmin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgedmin commented Feb 7, 2023

mgedmin commented Feb 7, 2023

gpongelli commented Jan 24, 2023 •

edited

Loading

mgedmin Jan 24, 2023 •

edited

Loading

gpongelli Jan 24, 2023 •

edited

Loading