Opened 13 years ago

Closed 12 years ago

Last modified 12 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: dev
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 13 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 by Aymeric Augustin, 13 years ago

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

comment:2 by Preston Holmes, 13 years ago

Easy pickings: set
Triage Stage: Unreviewed → Accepted

by Christopher Medrela, 13 years ago

Attachment: 17257.diff​ added

comment:3 by Christopher Medrela, 13 years ago

Has patch: set
Owner: changed from nobody to Christopher Medrela
Status: new → assigned

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

comment:4 by Aymeric Augustin, 13 years ago

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 by Christopher Medrela, 13 years ago

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 13 years ago by Christopher Medrela (next)

comment:6 by Christopher Medrela, 13 years ago

Owner: Christopher Medrela removed
Status: assigned → new

comment:7 by Kenneth Belitzky, 13 years ago

Owner: set to Kenneth Belitzky
Status: new → assigned

Pull request made: ​https://github.com/django/django/pull/63

comment:8 by Kenneth Belitzky, 13 years ago

Triage Stage: Accepted → Ready for checkin

comment:9 by Aymeric Augustin, 13 years ago

Triage Stage: Ready for checkin → Accepted

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

in reply to:  9 comment:10 by Kenneth Belitzky, 13 years ago

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 by Jannis Leidel, 12 years ago

Triage Stage: Accepted → Ready for checkin

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

Resolution: → fixed
Status: assigned → closed

In [8dafd04c45a66ff60f0503da1ebea14628322b45]:

Fixed #17257 - Removed outdated comment in syndication view

Thanks krzysiumed for the patch.

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

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 by Claude Paroz, 12 years ago

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