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

Improve python version formatting 🐍 affects [pypi piwheels github] #7682

Merged
merged 1 commit into from Mar 6, 2022

Conversation

chris48s
Copy link
Member

@chris48s chris48s commented Mar 4, 2022

After implementing the piwheels badge the other day I realised that I had just used renderVersionBadge() but our standard version formatter

if (first === '0' || /alpha|beta|snapshot|dev|pre/i.test(version)) {
return 'orange'
} else {
return 'blue'
}

isn't great for python packages because it will format something like 2.0.0rc1 or 2.0.0b2 as "stable".

Then I started pulling the thread and realised this problem actually also applies to some other python ecosystem badges.

so in this PR I have:

  • Added a pep440VersionColor() formatter function which uses https://github.com/renovatebot/pep440 to parse versions
  • Added the ability to pass renderVersionBadge() a custom formatter function as an optional param - this might be handy if we want to use a custom formatter for other package ecosystems.
  • Used this in the badges for pypi, piwheels and github-pipenv

@chris48s chris48s added the service-badge Accepted and actionable changes, features, and bugs label Mar 4, 2022
@shields-ci
Copy link

shields-ci commented Mar 4, 2022

Warnings
⚠️ This PR modified service code for pypi but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for piwheels but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for github but not its test code.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against cd0095f

PyvesB
PyvesB previously approved these changes Mar 5, 2022
Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍🏻 Interestingly , @paulmelnikow intended on using pep440 in #5592 as well.

Comment on lines 149 to 153
return {
label: dependency,
message: version ? addv(version) : ref,
color: 'blue',
color: version ? pep440VersionColor(version) : 'blue',
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we use renderVersionBadge here, either with a conditional or ternary operator? For example:

return version ? renderVersionBadge({ version, versionFormatter: pep440VersionColor }) 
               : { label: dependency, message: ref, color: 'blue' }

Copy link
Member Author

Choose a reason for hiding this comment

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

We'd have to extend renderVersionBadge() to also take a custom label. For this badge the label is the name of the locked dependency. Tbh I'm inclined to leave it

@shields-cd shields-cd temporarily deployed to shields-staging-pr-7682 March 6, 2022 17:50 Inactive
@chris48s
Copy link
Member Author

chris48s commented Mar 6, 2022

I've rebased this to resolve the conflicts on package.json and package-lock.json as it was easier than doing it as a merge. No functional changes

@chris48s chris48s merged commit 9186f0d into badges:master Mar 6, 2022
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.

None yet

4 participants