Noissue - Adding support for tel link scheme in Source.resolve_link of CMS

Noissue - Adding support for tel link scheme in Source.resolve_link of CMS

COLLABORATOR=vasily@adblockplus.org

[Vasily Kuznetsov, 2017-02-23 19:03:29]

https://codereview.adblockplus.org/29376948/diff/29376951/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29376948/diff/29376951/cms/sources.py#newc...
cms/sources.py:32: # manually testing tel scheme because it is not recoginzed by
urlparse
I think it would be better to make this special case a separate if statement.
This would avoid uglier line formatting to fit into 80 character limit and also
emphasize that this is a workaround. How about doing something like:

    if url.startswith('tel:'):
        # Workaround for 'tel' scheme that is not recognized on some platforms.
        return None, None

[juliandoucette, 2017-02-24 14:01:16]

> I think it would be better to make this special case a separate if statement.
> This would avoid uglier line formatting to fit into 80 character limit and
also
> emphasize that this is a workaround. How about doing something like:
> 
>     if url.startswith('tel:'):
>         # Workaround for 'tel' scheme that is not recognized on some
platforms.
>         return None, None

LGTM

I added you as a collaborator. I would mind taking this one?

[Vasily Kuznetsov, 2017-02-24 14:17:40]

On 2017/02/24 14:01:16, juliandoucette wrote:
> > I think it would be better to make this special case a separate if
statement.
> > This would avoid uglier line formatting to fit into 80 character limit and
> also
> > emphasize that this is a workaround. How about doing something like:
> > 
> >     if url.startswith('tel:'):
> >         # Workaround for 'tel' scheme that is not recognized on some
> platforms.
> >         return None, None
> 
> LGTM
> 
> I added you as a collaborator. I would mind taking this one?

I've uploaded a patch set with my proposed change. I've also added Sebastian as
a reviewer so I'm not reviewing my own code.

Sebastian, this review fixes the following problem: in some versions of Python
(at least 2.7.3 on one of our severs seems to do this) urlparse.urlparse desn't
recognize tel: scheme in URLs. This leads to warnings being produced for such
URLs and that unnecessarily pollutes alerts mailboxes. Since we can't avoid the
old Python versions that have this problem for now, we're adding this workaround
to silence the warning.

[Sebastian Noack, 2017-02-24 14:32:08]

https://codereview.adblockplus.org/29376948/diff/29376993/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29376948/diff/29376993/cms/sources.py#newc...
cms/sources.py:37: # Workaround for 'tel' scheme not recognized on some
platforms.
If this is related to the Python version, as you said, then this comment should
clarify that. So that we don't forget to remove this code, once we no longer
need to support Python 2, and strip out legacy code.

Also, there should probably be a regression test.

[Vasily Kuznetsov, 2017-02-24 16:48:09]

https://codereview.adblockplus.org/29376948/diff/29376993/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29376948/diff/29376993/cms/sources.py#newc...
cms/sources.py:37: # Workaround for 'tel' scheme not recognized on some
platforms.
On 2017/02/24 14:32:08, Sebastian Noack wrote:
> If this is related to the Python version, as you said, then this comment
should
> clarify that. So that we don't forget to remove this code, once we no longer
> need to support Python 2, and strip out legacy code.
Done.

> Also, there should probably be a regression test.
As we discussed over IRC, the test would not be useful since on Python >2.7.3
the workaround would not run and it's unlikely that developers will run the
tests on older versions of Python.

[Sebastian Noack, 2017-02-24 16:50:29]

LGTM