Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed #14656 -- Atom1Feed should write atom:published element #1366

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

Matt-Deacalion commented Jul 18, 2013

https://code.djangoproject.com/ticket/14656

The first commit – e51e589 was some housekeeping because I was in the neighbourhood… if it's cluttering the history I'll remove it. Cheers. 🐵

@timgraham timgraham commented on an outdated diff Jul 18, 2013

django/utils/feedgenerator.py
"""
- updates = [i['pubdate'] for i in self.items if i['pubdate'] is not None]
- if len(updates) > 0:
- updates.sort()
- return updates[-1]
+ updated = [i['updateddate'] for i in self.items
@timgraham

timgraham Jul 18, 2013

Owner

should this consider both updateteddate and pubdate together? i.e. right now if entry A has updatedated date yesterday and entry B has pubdate today, it looks like entry A will be considered the latest.

@timgraham timgraham commented on an outdated diff Jul 18, 2013

django/utils/feedgenerator.py
@@ -112,13 +112,14 @@ def __init__(self, title, link, description, language=None, author_email=None,
self.items = []
def add_item(self, title, link, description, author_email=None,
- author_name=None, author_link=None, pubdate=None, comments=None,
- unique_id=None, unique_id_is_permalink=None, enclosure=None,
- categories=(), item_copyright=None, ttl=None, **kwargs):
+ author_name=None, author_link=None, pubdate=None,
+ updateddate=None, comments=None, unique_id=None,
@timgraham

timgraham Jul 18, 2013

Owner

not listing updateddate as the last kwarg is technically backwards incompatible if someone is calling this item using args instead of kwargs but it seems unlikely to me.

@timgraham timgraham commented on an outdated diff Jul 18, 2013

docs/ref/contrib/syndication.txt
@@ -922,6 +940,7 @@ They share this interface:
* ``author_name``
* ``author_link``
* ``pubdate``
+ * ``updateddate``
@timgraham

timgraham Jul 18, 2013

Owner

there should be a .. versionadded:: 1.7 somewhere here that notes when updateddate was added

@timgraham timgraham commented on an outdated diff Jul 18, 2013

docs/ref/utils.txt
@@ -342,11 +342,11 @@ SyndicationFeed
All parameters should be Unicode objects, except ``categories``, which
should be a sequence of Unicode objects.
- .. method:: add_item(title, link, description, [author_email=None, author_name=None, author_link=None, pubdate=None, comments=None, unique_id=None, enclosure=None, categories=(), item_copyright=None, ttl=None, **kwargs])
+ .. method:: add_item(title, link, description, [author_email=None, author_name=None, author_link=None, pubdate=None, updateddate=None, comments=None, unique_id=None, enclosure=None, categories=(), item_copyright=None, ttl=None, **kwargs])
@timgraham

timgraham Jul 18, 2013

Owner

same as above, needs a versionadded

@timgraham timgraham commented on an outdated diff Jul 18, 2013

tests/syndication/tests.py
@@ -257,12 +356,14 @@ def test_aware_datetime_conversion(self):
"""
response = self.client.get('/syndication/aware-dates/')
doc = minidom.parseString(response.content)
- updated = doc.getElementsByTagName('updated')[0].firstChild.wholeText
- self.assertEqual(updated[-6:], '+00:42')
+ published = doc.getElementsByTagName('published')[0].\
@timgraham

timgraham Jul 18, 2013

Owner

I think I'd prefer not to make the PEP8 changes, the existing style seems fine to me.

Contributor

Matt-Deacalion commented Jul 18, 2013

Thanks for the feedback Tim, I've made the changes you suggested.

Fixed #14656 -- Atom1Feed `published` element
Some feed aggregators make use of the `published` element as well as
the `updated` element (within the Atom standard -- http://bit.ly/2YySb).

The standard allows for these two elements to be present in the same
entry. `Atom1Feed` had implemented the `updated` element which was
incorrectly taking the date from `pubdate`.

@timgraham timgraham commented on an outdated diff Jul 19, 2013

django/utils/feedgenerator.py
"""
- updates = [i['pubdate'] for i in self.items if i['pubdate'] is not None]
- if len(updates) > 0:
- updates.sort()
- return updates[-1]
+ dates = []
+
+ for item in self.items:
+ for k, v in item.iteritems():
@timgraham

timgraham Jul 19, 2013

Owner

I don't think there's a need to iterate through all the items values. Just check if the two we're interested in exist?

Also, could you add a test for the issue I pointed out that's been fixed here?

Contributor

Matt-Deacalion commented Jul 19, 2013

I've updated the latest_post_date method, it doesn't iterate through all the values now. I've also added a test for this method. Thanks for the code review. :-)

Owner

timgraham commented Jul 19, 2013

made some final tweaks and merged in a269ea4 - thanks Matt!

@timgraham timgraham closed this Jul 19, 2013

Contributor

Matt-Deacalion commented Jul 19, 2013

Great, nice changes in latest_post_date!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment