Issue 3435 - strip "index" from page path for canonical URL

Issue 3435 - strip "index" from page path for canonical URL

[saroyanm, 2015-12-18 10:29:52]

@Sebastian can you please have a look when you have time.

[Wladimir Palant, 2015-12-18 10:58:15]

https://codereview.adblockplus.org/29332874/diff/29332875/filters/stripindex.py
File filters/stripindex.py (right):

https://codereview.adblockplus.org/29332874/diff/29332875/filters/stripindex....
filters/stripindex.py:3: return path[:-5]
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]

New patch uploaded.

https://codereview.adblockplus.org/29332874/diff/29332875/filters/stripindex.py
File filters/stripindex.py (right):

https://codereview.adblockplus.org/29332874/diff/29332875/filters/stripindex....
filters/stripindex.py:3: return path[:-5]
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]

https://codereview.adblockplus.org/29332874/diff/29332875/filters/stripindex.py
File filters/stripindex.py (right):

https://codereview.adblockplus.org/29332874/diff/29332875/filters/stripindex....
filters/stripindex.py:3: return path[:-5]
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:19]

https://codereview.adblockplus.org/29332874/diff/29332875/filters/stripindex.py
File filters/stripindex.py (right):

https://codereview.adblockplus.org/29332874/diff/29332875/filters/stripindex....
filters/stripindex.py:3: return path[:-5]
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:32]

LGTM

https://codereview.adblockplus.org/29332874/diff/29332875/filters/stripindex.py
File filters/stripindex.py (right):

https://codereview.adblockplus.org/29332874/diff/29332875/filters/stripindex....
filters/stripindex.py:3: return path[:-5]
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.