-
Notifications
You must be signed in to change notification settings - Fork 121
pip: Add changelog links from pypi to checker data #440
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
base: master
Are you sure you want to change the base?
Conversation
if len(keys) == 1: | ||
return urls[keys.pop()] | ||
elif len(keys) > 1: | ||
print(f"Warning: Got multiple potential keys for the changelog, picking one at random from {keys}.") | ||
return urls[keys.pop()] |
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'd just combine these two cases and pick the first one always. If it's wrong it needs to be manually edited anyways and the warning doesn't particularly mean that it was wrong either.
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 any data on how many projects might have values for more than one of those keys or if the keys may actually have different semantic meanings. (In some projects on Github I definitely saw differences in changlog and release notes.)
Therefore my intention was to give the users a hint that they might not get the best result or then one they are expecting. It could be rewritten to be more informational than a warning, maybe also the code cleaned up a bit. (Maybe printing all keys and values to be more useful and allow for easy manual fixing afterwards.)
But if you think it is really unnecessary and want the code as short as possible I'm happy to remove it.
Is this ready or not? If so you have to rebase and undraft. Also format it https://github.com/flatpak/flatpak-builder-tools/blob/master/pip/readme.md#development I think adding extra metadata to JSON is fine. |
return source['url'] | ||
raise Exception('Failed to extract url from {}'.format(url)) | ||
|
||
def get_pypi_changelog(name: str) -> str: |
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.
return type is wrong.
Thanks for the review and good comments, I'll implement them asap! It is not ready because I'm not sure about the naming of the key. In the beginning I assumed that it should be a template (to add the current version), therefore |
I don't have an opinion about the key name, either works I think. It's going to be generated anyways. |
8c5a0ec
to
ea9bfa2
Compare
Working on a feature for data checker to make changes easier to review by adding links to changelogs: flathub-infra/flatpak-external-data-checker#339
My proof of concept works fine already and it obviously makes sense to pull the changelog links from pypi. Let me know what you think, PR for the checker will follow.