Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#17257 closed Cleanup/optimization (fixed)

Comment that contradicts code in django.contrib.syndication.views

Reported by: Aymeric Augustin Owned by: Kenneth Belitzky
Component: contrib.syndication Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

        # 'url' must already be ASCII and URL-quoted, so no need for encoding
        # conversions here.
        url = iri_to_uri(u'%s://%s%s' % (protocol, domain, url))

But this line is a conversion!

This problem was introduced at r5629. I think we should just remove the comment.

Attachments (1)

17257.diff (582 bytes) - added by Christopher Medrela 5 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 years ago by Aymeric Augustin

Summary: Comments that contradicts code in django.contrib.syndication.viewsComment that contradicts code in django.contrib.syndication.views

comment:2 Changed 5 years ago by Preston Holmes

Easy pickings: set
Triage Stage: UnreviewedAccepted

Changed 5 years ago by Christopher Medrela

Attachment: 17257.diff added

comment:3 Changed 5 years ago by Christopher Medrela

Has patch: set
Owner: changed from nobody to Christopher Medrela
Status: newassigned

Note that file django/contrib/syndication/feeds.py was removed. I edited file django/contrib/syndication/views.py.

comment:4 Changed 5 years ago by Aymeric Augustin

Do you have some evidence that removing the comment is actually the right thing to do?

I didn't commit the fix right away because I wasn't sure of that.

comment:5 Changed 5 years ago by Christopher Medrela

No, I have not. I thought you are sure, but you didn't commit the fix because you want to leave this easy ticket for newcomers.

Version 0, edited 5 years ago by Christopher Medrela (next)

comment:6 Changed 5 years ago by Christopher Medrela

Owner: Christopher Medrela deleted
Status: assignednew

comment:7 Changed 4 years ago by Kenneth Belitzky

Owner: set to Kenneth Belitzky
Status: newassigned

comment:8 Changed 4 years ago by Kenneth Belitzky

Triage Stage: AcceptedReady for checkin

comment:9 Changed 4 years ago by Aymeric Augustin

Triage Stage: Ready for checkinAccepted

Please don't mark your own patches as ready for checkin -- see the triage guidelines for more information.

comment:10 in reply to:  9 Changed 4 years ago by Kenneth Belitzky

Replying to aaugustin:

Please don't mark your own patches as ready for checkin -- see the triage guidelines for more information.

oops ... sorry! i'll check on that.

is there any doc that defines how workflow works with github? there's one wiki page which talks about this but ends up generating a patch. will this be the only solution to contribution? will pull requests get any attention ?

comment:11 Changed 4 years ago by Jannis Leidel

Triage Stage: AcceptedReady for checkin

comment:12 Changed 4 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: assignedclosed

In [8dafd04c45a66ff60f0503da1ebea14628322b45]:

Fixed #17257 - Removed outdated comment in syndication view

Thanks krzysiumed for the patch.

comment:13 in reply to:  12 Changed 4 years ago by Kenneth Belitzky

this same patch was contributed by me 2 months ago with a pull request https://github.com/django/django/pull/63 ...
what kind of workflow are you using?

Replying to Claude Paroz <claude@…>:

In [8dafd04c45a66ff60f0503da1ebea14628322b45]:

Fixed #17257 - Removed outdated comment in syndication view

Thanks krzysiumed for the patch.

comment:14 Changed 4 years ago by Claude Paroz

If there's a patch attached, no need to add an identical pull request. There is no more value (and no less) in a pull request than an attached patch. Personally, I will always push the patch myself until Github supports fast-forward merges.

Note: See TracTickets for help on using tickets.
Back to Top