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 [PythonVersionFromToml] shield #9516

Merged
merged 6 commits into from Sep 10, 2023

Conversation

jNullj
Copy link
Collaborator

@jNullj jNullj commented Aug 25, 2023

Added new shield PythonVersionFromToml
The shield display required python versions for packages based on pyproject.toml
Example:
For URL https://raw.githubusercontent.com/numpy/numpy/main/pyproject.toml
Shield URL is: http://localhost:8080/python/required-version-toml?tomlFilePath=https%3A%2F%2Fraw.githubusercontent.com%2Fnumpy%2Fnumpy%2Fmain%2Fpyproject.toml
And the result:
image

Tester is also added in this PR.

Fixes #9410

Added new shield per issue badges#9410
The shield display required python versions for packages based on pyproject.toml
@github-actions
Copy link
Contributor

github-actions bot commented Aug 25, 2023

Messages
📖 ✨ Thanks for your contribution to Shields, @jNullj!

Generated by 🚫 dangerJS against 4e86c49

@jNullj jNullj marked this pull request as ready for review August 25, 2023 18:05
@calebcartwright calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Aug 27, 2023

static examples = [
{
title: 'Python Required Version TOML',
Copy link
Member

Choose a reason for hiding this comment

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

Can we clarify in the title and description that this is for PEP 621-compliant pyproject.toml

Python packaging is..fun 😬 and lots of projects have a pyproject.toml that doesn't use this format e.g: https://github.com/chris48s/arcgis2geojson/blob/master/pyproject.toml#L12

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed with e2ecbf5

Comment on lines +26 to +28
base: '',
pattern: 'python/required-version-toml',
queryParamSchema,
Copy link
Member

Choose a reason for hiding this comment

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

I don't have an answer for this at the moment, but I'm wondering where this should live.

Python isn't really a "service". Neither is "pyproject.toml".

We've got a bunch of other badges knocking about under /github that just read a file on GitHub. We've talked before about converting them to work like this so you can just pass a URL in as a query param. Then they could work for files hosted elsewhere. I wonder if it makes sense to lump them together as a "service".

I need to have a think about this one. Any other maintainers got a view on what a sensible route would be for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was not sure myself, i used SecurityHeaders as reference and saw it used an empty base route.
What should we do with that?

Copy link
Member

Choose a reason for hiding this comment

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

So it isn't really about the empty base. It is more that our URL schema assumes the first part of the route is a service, but there's no service here.
Given nobody else has chimed in on this, I think I'm going to merge this but note this as a consideration in #5343

Comment on lines 5 to 12
const isCommaSeparatedPythonVersions = Joi.string().regex(
// This should test for PEP440
// Accepted values are one or more Version specifiers as defined at https://peps.python.org/pep-0440/#version-specifiers
// Some strings might include spaces, those are valid, values are comma seperated
// Versions should fit the version scheme https://peps.python.org/pep-0440/#version-scheme
// This is based on the example in PEP440 at https://peps.python.org/pep-0440/#appendix-b-parsing-version-strings-with-regular-expressions
/^\s*(?:(?:===|!=|<=|>=|<|>)\s*)?((?:(?:\d+!)?(?:\d+(?:\.\d+)*))(?:(?:[abc]|rc)\d+)?(?:\.post\d+)?(?:\.dev\d+)?)(?:\s*,\s*(?:(?:===|!=|<=|>=|<|>)\s*)?((?:(?:\d+!)?(?:\d+(?:\.\d+)*))(?:(?:[abc]|rc)\d+)?(?:\.post\d+)?(?:\.dev\d+)?))*\s*$/,
)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than copy/pasing this monster regex (I hope you didn't write this from scratch! 😄 ), I'd suggest we use Joi.custom() https://joi.dev/api/#anycustommethod-description here to make a validator using the validRange() function from @renovate/pep440 (which is a package we already use).

import pep440 from '@renovate/pep440'
pep440.validRange(">=3.6, <4.0") // true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be fixed with b3de30f
I still have issue testing so i feel like mountain climbing where i try to get it first try, lets see the CI results.

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

Solves review badges#9516 (comment)
@jNullj jNullj requested a review from chris48s September 5, 2023 21:51
@github-actions
Copy link
Contributor

🚀 Updated review app: https://pr-9516-badges-shields.fly.dev

@chris48s chris48s added this pull request to the merge queue Sep 10, 2023
Merged via the queue into badges:master with commit e8b4467 Sep 10, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python versions from pyproject.toml
3 participants