-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Changes from 3 commits
43a171b
97b418d
05702d5
e2ecbf5
b3de30f
4e86c49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
import Joi from 'joi' | ||
import BaseTomlService from '../../core/base-service/base-toml.js' | ||
import { optionalUrl } from '../validators.js' | ||
|
||
const queryParamSchema = Joi.object({ | ||
tomlFilePath: optionalUrl.required(), | ||
}).required() | ||
|
||
const schema = Joi.object({ | ||
project: Joi.object({ | ||
'requires-python': Joi.string().required(), | ||
}).required(), | ||
}).required() | ||
|
||
const documentation = `Shows the required python version for a package based on the values in the requires-python field in the pyproject.toml \n | ||
a URL of the toml is required, please note that when linking to files in github or similar sites, provide URL to raw file, for example: | ||
|
||
Use https://raw.githubusercontent.com/numpy/numpy/main/pyproject.toml \n | ||
And not https://github.com/numpy/numpy/blob/main/pyproject.toml | ||
` | ||
|
||
class PythonVersionFromToml extends BaseTomlService { | ||
static category = 'platform-support' | ||
|
||
static route = { | ||
base: '', | ||
pattern: 'python/required-version-toml', | ||
queryParamSchema, | ||
} | ||
|
||
static examples = [ | ||
{ | ||
title: 'Python Required Version TOML', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed with e2ecbf5 |
||
namedParams: {}, | ||
queryParams: { | ||
tomlFilePath: | ||
'https://raw.githubusercontent.com/numpy/numpy/main/pyproject.toml', | ||
}, | ||
staticPreview: this.render({ requiresPythonString: '>=3.9' }), | ||
documentation, | ||
}, | ||
] | ||
|
||
static defaultBadgeData = { label: 'python' } | ||
|
||
static render({ requiresPythonString }) { | ||
// we only show requries-python as is | ||
// for more info read the following issues: | ||
// https://github.com/badges/shields/issues/9410 | ||
// https://github.com/badges/shields/issues/5551 | ||
return { | ||
message: requiresPythonString, | ||
color: 'blue', | ||
} | ||
} | ||
|
||
async handle(namedParams, { tomlFilePath }) { | ||
const tomlData = await this._requestToml({ url: tomlFilePath, schema }) | ||
const requiresPythonString = tomlData.project['requires-python'] | ||
|
||
return this.constructor.render({ requiresPythonString }) | ||
} | ||
} | ||
|
||
export { PythonVersionFromToml } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import Joi from 'joi' | ||
import { createServiceTester } from '../tester.js' | ||
export const t = await createServiceTester() | ||
|
||
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*$/, | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 import pep440 from '@renovate/pep440'
pep440.validRange(">=3.6, <4.0") // true There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be fixed with b3de30f |
||
|
||
t.create('python versions (valid)') | ||
.get( | ||
'/python/required-version-toml.json?tomlFilePath=https://raw.githubusercontent.com/numpy/numpy/main/pyproject.toml', | ||
) | ||
.expectBadge({ label: 'python', message: isCommaSeparatedPythonVersions }) | ||
|
||
t.create( | ||
'python versions - valid toml with missing python-requires field (invalid)', | ||
) | ||
.get( | ||
'/python/required-version-toml.json?tomlFilePath=https://raw.githubusercontent.com/django/django/main/pyproject.toml', | ||
) | ||
.expectBadge({ label: 'python', message: 'invalid response data' }) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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