filters/stripindex.py - Issue 29332874: Issue 3435 - strip "index" from page path for canonical URL

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

Delta Between Two Patch Sets: filters/stripindex.py

Issue 29332874: Issue 3435 - strip "index" from page path for canonical URL (Closed)

Left Patch Set: Created Dec. 18, 2015, 10:28 a.m.

Right Patch Set: Went with Wladimir suggestion Created Dec. 18, 2015, 11:08 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 def stripindex(path):	1 def stripindex(path):

2 if path.endswith("index"):	2 parts = path.split("/")

3 return path[:-5]	3 if parts[-1] == "index":
Wladimir Palant 2015/12/18 10:58:15 This logic is wrong - imagine a page called fooind This logic is wrong - imagine a page called fooindex. I think it should be something along the lines of: parts = path.split("/") if parts[-1] == "index": parts.pop() return "/".join(parts) saroyanm 2015/12/18 11:10:15 Ahh right. Show quoted text On 2015/12/18 10:58:15, Wladimir Palant wrote: > This logic is wrong - imagine a page called fooindex. > > I think it should be something along the lines of: > > parts = path.split("/") > if parts[-1] == "index": > parts.pop() > return "/".join(parts) Ahh right. Wladimir Palant 2015/12/18 11:13:44 In fact, given that this logic is only necessary i In fact, given that this logic is only necessary in exactly one place, maybe it is better to put it directly into the template: {# Drop "index" at the end of page path #} {% set canonical = page.split("/") %} {% if canonical[-1] == "index" %} {% set _ = canonical.pop() %} {% endif %} <link rel="canonical" href="https://adblockplus.org/{{ canonical\|join("/")}}"> saroyanm 2015/12/18 11:24:18 IMO it will affect readability of template file, I Show quoted text On 2015/12/18 11:13:44, Wladimir Palant wrote: > In fact, given that this logic is only necessary in exactly one place, maybe it > is better to put it directly into the template: > > {# Drop "index" at the end of page path #} > {% set canonical = page.split("/") %} > {% if canonical[-1] == "index" %} > {% set _ = canonical.pop() %} > {% endif %} > <link rel="canonical" href="https://adblockplus.org/{{ canonical\|join("/")}}"> IMO it will affect readability of template file, I think this implementation doesn't belong to template. I'd vote for keeping it separate, but will move to template if you insist. Wladimir Palant 2015/12/18 11:58:31 Fair enough. Show quoted text On 2015/12/18 11:24:18, saroyanm wrote: > IMO it will affect readability of template file, I think this implementation > doesn't belong to template. I'd vote for keeping it separate, but will move to > template if you insist. Fair enough.
4 return path	4 parts.pop()

5	5 return "/".join(parts)

LEFT	RIGHT