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

Package update feed #2165

Closed
wants to merge 10 commits into from
Closed

Conversation

AyumuKasuga
Copy link
Contributor

@AyumuKasuga AyumuKasuga commented Jul 2, 2017

Fixes #2551, fixes #1683.
Url like /rss/{package_name}/updates.xml with last 10 releases.

I hope it solves pypi#1683 issue.
Url like `/rss/{package_name}/updates.xml` with last 10 releases.
@nlhkabu
Copy link
Contributor

nlhkabu commented Jul 25, 2017

Hi @AyumuKasuga - thanks for this PR.
Can you please provide a screenshot of each of the template updates so I can validate? Thanks

@AyumuKasuga
Copy link
Contributor Author

Hi @nlhkabu
There is no visual changes in templates in this PR, it's only adds <link> tag in template here

And RSS feed is just xml like this:
localhost-rss-eniarbiter-updates xml

@@ -0,0 +1,14 @@
{% extends "base.xml" %}
{% block title %}Recent Updates of {{ project_name }} package{% endblock %}
{% block description %}Recent Updates of {{ project_name }} to the Python Package Index{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

It might be better if this title and description said something like "Latest releases of the {{ project_name }} package".

At the very least, this should say "updates to the {{ project_name }} package" instead of "updates of {{ project_name }} package"

@@ -56,6 +56,10 @@ <h3 class="package-snippet__title">

{% block description %}{{ release.summary }}{% endblock %}

{% block extra_rss %}
<link rel="alternate" type="application/rss+xml" title="RSS: updates of {{ release.project.normalized_name }}" href="{{ request.route_path('rss.project_updates', name=release.project.normalized_name) }}">
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here about "updates of".

)

if not latest_releases:
return HTTPNotFound()
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a difference here between the case when project hasn't been registered yet, and the case when the project exists, but doesn't have any releases.

For the former, I would expect a 404, but for the latter, I would expect an RSS feed with no <item>s.

This would be handled by using traverse as mentioned in my other comment.

@@ -141,6 +141,8 @@ def add_policy(name, filename):
"https://files.example.com/packages/{path}",
),
pretend.call("rss.updates", "/rss/updates.xml", domain=warehouse),
pretend.call("rss.project_updates",
"/rss/{name}/updates.xml", domain=warehouse),
Copy link
Member

Choose a reason for hiding this comment

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

If you set the following:

traverse="/{name}",
factory="warehouse.packaging.models:ProjectFactory",

you can expect a project argument to the rss_project_updates function, and a query will be unnecessary. This will also handle 404-ing if the project doesn't exist.

return {
"project_name": project_name,
"latest_releases": latest_releases
}
Copy link
Member

Choose a reason for hiding this comment

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

If we made the necessary changes to get a project via the ProjectFactory, we could just pass the project variable to the template here, and use project.name and project.releases (which is ordered by creation date) directly in the template.

@AyumuKasuga
Copy link
Contributor Author

Hello @di
Thanks for the review. I changed templates and improved project_updates view using ProjectFactory

@@ -0,0 +1,14 @@
{% extends "base.xml" %}
{% block title %}Latest releases of the {{ project.name }} package{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: this should probably say "project", not package. It would be good to put the project name in quotes too.

@@ -0,0 +1,14 @@
{% extends "base.xml" %}
{% block title %}Latest releases of the {{ project.name }} package{% endblock %}
{% block description %}Latest releases of the {{ project.name }} to the Python Package Index{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

Same here, let's add quotes and "project".

@AyumuKasuga
Copy link
Contributor Author

@di thanks, fixed!

di
di previously approved these changes Jul 28, 2017
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jakirkham
Copy link

Anything else needed for this PR? Seems very useful (though I'm biased 😄).

@rce
Copy link

rce commented Feb 2, 2018

Hello @di! Is there something preventing merge and can I do something to make it happen?

@di
Copy link
Member

di commented Feb 2, 2018

Yes, sorry for not following up. I had some reservations about this after approving, but haven't had a chance to address them (my time has mostly been spent in favor of development which helps fully us transition from pypi-legacy to Warehouse), so this PR has been on the backburner.

One thing: I wanted to check/ensure that this view is either not cached, or purged when a new release is made.

Another thing is that right now it's possible to delete releases (via pypi-legacy, and soon via Warehouse as well) and it would be ideal to have an entry in this feed for when a release is deleted as well. As it stands right now, the feed introduced by this PR just shows all the existing releases for a project. It might make more sense to look at the Journal entries for a project instead, which would include new releases and deletions.

I don't really want to start @AyumuKasuga off on revising this PR until I've really had a chance to think about it again (and other PyPI maintainers have weighed in) but perhaps they could do a little investigation of those ideas and report back.

@di di dismissed their stale review February 2, 2018 22:42

Still have some things to figure out.

@brainwane brainwane added this to the Cool but not urgent milestone Feb 5, 2018
@brainwane
Copy link
Contributor

I'm also sorry for the slow response!

To second Dustin: The folks working on Warehouse have gotten funding to concentrate on improving and deploying Warehouse, and have kicked off work towards our development roadmap -- the most urgent task is, as Dustin said, to improve Warehouse to the point where we can redirect pypi.python.org to pypi.org so the site is more sustainable and reliable. Since this feature isn't something that the legacy site has, I've moved it to a future milestone. But Dustin, Ernest, and other developers are interested in giving @AyumuKasuga feedback as they proceed, here or in IRC.

@di mentioned that the right approach might be to provide a feed of Journal updates -- @AyumuKasuga, you should check out #2871 to see that work progress, and maybe build on that!

Thanks and sorry again for the wait.

@brainwane
Copy link
Contributor

@AyumuKasuga Now that #2871 is finished, would you like to return to this PR and talk with us about how to proceed? Perhaps using the Journal entries?

As always, if you have questions along the way as you work on this, please feel free to ask them here, in #pypa-dev on Freenode, or the pypa-dev mailing list. Thank you!

@brainwane
Copy link
Contributor

@AyumuKasuga Ping :)

@AyumuKasuga
Copy link
Contributor Author

AyumuKasuga commented Apr 18, 2018

@brainwane sorry, currently don't have time to work on this.

@di
Copy link
Member

di commented Apr 24, 2018

No worries @AyumuKasuga. Given that, I'm going to close this PR in favor of a different, yet-to-be-implemented API (see #284) which will ideally have the features I mentioned in #2165 (comment).

Anyone that was hoping for this to be merged: Feel free to file a new issue with your specific use-case and we can try to help you make do with the existing APIs (this will also help us consider your use-case when creating the new API).

@di di closed this Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RSS feed for new file uploads to existing packages Package update feeds
6 participants