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

Python versions from pyproject.toml #9410

Closed
HealthyPear opened this issue Jul 25, 2023 · 6 comments · Fixed by #9516
Closed

Python versions from pyproject.toml #9410

HealthyPear opened this issue Jul 25, 2023 · 6 comments · Fixed by #9516
Assignees
Labels
good first issue New contributors, join in! service-badge New or updated service badge

Comments

@HealthyPear
Copy link

📋 Description

  • service-independent
  • should show required range of python versions same as the one for PyPI

🔗 Data

any pyproject.toml file, from the requires-python field

🎤 Motivation

should be even easier than the one on PyPI and it will work as a dynamic badge for people who are working on packages that are either public or private, but not a public package

@HealthyPear HealthyPear added the service-badge New or updated service badge label Jul 25, 2023
@chris48s
Copy link
Member

chris48s commented Jul 25, 2023

Couple of notes:

This would be for a PEP 621-compliant pyproject.toml Not all packageing tools follow the spec. For example, poetry stores this in tool.poetry.dependencies.python, rather than project.requires-python

Currently shields has base classes for dealing with requests that return JSON, XML and Yaml. We don't have one for Toml so part of the job would be to add a toml parser and base class for it. This is not a blocker and would be useful to have anyway, but we don't currently have it as a building block.

@chris48s chris48s added the good first issue New contributors, join in! label Jul 25, 2023
@jNullj
Copy link
Contributor

jNullj commented Jul 28, 2023

Hey, i could help.
I guess there is no point in writing a parser from scratch just like we use parsers from other modules for yaml, xml etc...
The toml site refers to a 3 packages that are complaint with tomlv1.0.0 (latest):

name
smol-toml
toml-nodejs
@ltd/j-toml

smol-toml is relatively new but seems easy to use, boasts big performance and appears to be easier to implement.

All of those are hardly in use thats why i also took a look at packages that don't comply with latest toml spec.
There is also toml which is way more in use (900k weekly downloads, 240k github projects) but it only supports toml v0.4.0. could this be an issue? it seems to be very old, i fear newer nodejs versions might conflict with it in the future.

Should i got ahead with smol-toml, This would add a new dependency to the project.

@chris48s
Copy link
Member

Yeah we should definitely use a package and not write our own parser.


There is also toml which is way more in use (900k weekly downloads, 240k github projects) but it only supports toml v0.4.0. could this be an issue? it seems to be very old, i fear newer nodejs versions might conflict with it in the future.

Yes. This from the readme does not give me high confidence this lib is being actively maintained 😄

toml-node is tested on Travis CI and is tested against:

  • Node 0.10
  • Node 0.12
  • Latest stable io.js

Lets pass on this one.


Of the other packages you've suggested:

  • @ltd/j-toml seems to be the most popular
  • smol-toml requires node 18 minimum. I've been having trouble getting a review on deploy on node 18 #9385. I might just go rogue and merge this at the weekend. In any case, you can assume we'll be running node 18 in the near future, although current master is still on node 16

Having had a quick look at the parsed objects that those 2 parsers return, I think if we implemented this using smol-toml and had to switch it out for @ltd/j-toml at a later date, it should be possible to switch them pretty transparently. I think it would be reasonable to take a punt on smol-toml 👍

jNullj added a commit to jNullj/shields-fun-fork that referenced this issue Aug 4, 2023
Add base toml service to enable fetch of toml files
Add spec file for the new toml service for automated testing
This was added to allow a new way to retrive python version from pyproject.toml as described in issue badges#9410
jNullj added a commit to jNullj/shields-fun-fork that referenced this issue Aug 4, 2023
Add base toml service to enable fetch of toml files
Add spec file for the new toml service for automated testing
This was added to allow a new way to retrive python version from pyproject.toml as described in issue badges#9410
jNullj added a commit to jNullj/shields-fun-fork that referenced this issue Aug 4, 2023
Add base toml service to enable fetch of toml files
Add spec file for the new toml service for automated testing
This was added to allow a new way to retrive python version from pyproject.toml as described in issue badges#9410
jNullj added a commit to jNullj/shields-fun-fork that referenced this issue Aug 10, 2023
Add base toml service to enable fetch of toml files
Add spec file for the new toml service for automated testing
This was added to allow a new way to retrive python version from pyproject.toml as described in issue badges#9410
github-merge-queue bot pushed a commit that referenced this issue Aug 16, 2023
* Add TOML support with [BaseTomlService]

Add base toml service to enable fetch of toml files
Add spec file for the new toml service for automated testing
This was added to allow a new way to retrive python version from pyproject.toml as described in issue #9410

* Fix typo

Co-authored-by: chris48s <chris48s@users.noreply.github.com>

* refactor: improve code readability

solve code review #9438 (comment)

---------

Co-authored-by: chris48s <chris48s@users.noreply.github.com>
@jNullj
Copy link
Contributor

jNullj commented Aug 16, 2023

Now that we got TOML support I can add this, I hope i can get it done this weekend

@jNullj
Copy link
Contributor

jNullj commented Aug 23, 2023

While working on the new shield I realized that unlike PyPI, the requires-python field in the pyproject.toml returns the versions in a format of operators-version (e.g ">=3.1,!=3.3,....")
While i could add logic to handle that I will have to set the latest versions with a call to another API.

  1. @chris48s do you know if we already cache something like latest python 3/2 minor version?
  2. @HealthyPear @chris48s if a call is required would it be better to make an API call to fetch latest versions then cache it for a long time or should I change the shield's format to something similar to '>= 3.5' or '3.1-latest'?

I think that using a static value for latest python versions should be avoided as it can cause confusion and show inaccurate results.

@chris48s
Copy link
Member

I think for this badge (pulling direct from a pyproject.toml), we should just take the value of requires-python and put it on a badge. If we wanted to add one that presents a delimited list, we could add a variant that does that based on the trove classifiers, but we shouldn't try to derive it from requires-python IMO.

There is a really long thread at #5551 about how we should deal with this when we get this from the PyPI and whether we should use python_requires or classifiers. It ended up with us.. not changing anything.

jNullj added a commit to jNullj/shields-fun-fork that referenced this issue Aug 25, 2023
Added new shield per issue badges#9410
The shield display required python versions for packages based on pyproject.toml
jNullj added a commit to jNullj/shields-fun-fork that referenced this issue Aug 25, 2023
Added new shield per issue badges#9410
The shield display required python versions for packages based on pyproject.toml
github-merge-queue bot pushed a commit that referenced this issue Sep 10, 2023
* Add PythonVersionFromToml

Added new shield per issue #9410
The shield display required python versions for packages based on pyproject.toml

* Add tests for PythonVersionFromToml

* Improve docs for tests regex

* Improve title and description

Rename and updated description to bring into focus that only PEP 621 complaint pyproject.toml files are supported.

Solves review #9516 (comment)

* replace complex regex with @renovate/pep440

solves review #9516 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue New contributors, join in! service-badge New or updated service badge
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants