Code

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#17257 closed Cleanup/optimization (fixed)

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

Reported by: aaugustin Owned by: httpdss
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 krzysiumed 3 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by aaugustin

  • Summary changed from Comments that contradicts code in django.contrib.syndication.views to Comment that contradicts code in django.contrib.syndication.views

comment:2 Changed 3 years ago by ptone

  • Easy pickings set
  • Triage Stage changed from Unreviewed to Accepted

Changed 3 years ago by krzysiumed

comment:3 Changed 3 years ago by krzysiumed

  • Has patch set
  • Owner changed from nobody to krzysiumed
  • Status changed from new to assigned

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

comment:4 Changed 3 years ago by aaugustin

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 3 years ago by krzysiumed

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 3 years ago by krzysiumed (next)

comment:6 Changed 3 years ago by krzysiumed

  • Owner krzysiumed deleted
  • Status changed from assigned to new

comment:7 Changed 2 years ago by httpdss

  • Owner set to httpdss
  • Status changed from new to assigned

comment:8 Changed 2 years ago by httpdss

  • Triage Stage changed from Accepted to Ready for checkin

comment:9 follow-up: Changed 2 years ago by aaugustin

  • Triage Stage changed from Ready for checkin to Accepted

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 2 years ago by httpdss

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 2 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 follow-up: Changed 2 years ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In [8dafd04c45a66ff60f0503da1ebea14628322b45]:

Fixed #17257 - Removed outdated comment in syndication view

Thanks krzysiumed for the patch.

comment:13 in reply to: ↑ 12 Changed 2 years ago by httpdss

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 2 years ago by claudep

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.