Issue 7461 - Include source page into warning text for unresolved links

Issue 7461 - Include source page into warning text for unresolved links

Repository: https://hg.adblockplus.org/cms/
Base revision: 66c6722b1ca2

[Vasily Kuznetsov, 2019-04-12 14:38:34]

Hi Rosie,

This patch fixes the unhelpful warnings that the CMS produces when there's a
link to a page that doesn't exist.

Let me know what you think.

Cheers,
Vasily

[rhowell, 2019-04-12 20:17:12]

Hey Vasily!

Mostly LGTM. Just a couple small nits that I don't feel too strongly about. (See
below) If this is time-sensitive or anything, don't worry about em.

https://codereview.adblockplus.org/30044555/diff/30044559/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/30044555/diff/30044559/cms/sources.py#newc...
cms/sources.py:63: page, source_page)
NIT: It might be slightly easier to read the error message the other way around.
E.g.

Link from "rel_path" to "rel_path.html" cannot be resolved
Link from "rel_path" to "myscripts.js" cannot be resolved

https://codereview.adblockplus.org/30044555/diff/30044559/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/30044555/diff/30044559/tests/test_page_out...
tests/test_page_outputs.py:79: assert message in messages
NIT: This test might be a bit stricter if we used something like
`set(expected_messages) == messages`, but it seems there's two extra broken
links in there (that might've been my fault). Would it make sense to clean up
those broken links? It's these two:

Link to "rel_path.html" (from "rel_path") cannot be resolved
Link to "myscripts.js" (from "rel_path") cannot be resolved

[Vasily Kuznetsov, 2019-04-15 16:48:45]

Hi Rosie!

Thank you for the review. I have improved the error message. Let me know what
you think.

Cheers,
Vasily

https://codereview.adblockplus.org/30044555/diff/30044559/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/30044555/diff/30044559/cms/sources.py#newc...
cms/sources.py:63: page, source_page)
On 2019/04/12 20:17:12, rhowell wrote:
> NIT: It might be slightly easier to read the error message the other way
around.
> E.g.
> 
> Link from "rel_path" to "rel_path.html" cannot be resolved
> Link from "rel_path" to "myscripts.js" cannot be resolved

Yeah, makes sense. Done.

https://codereview.adblockplus.org/30044555/diff/30044559/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/30044555/diff/30044559/tests/test_page_out...
tests/test_page_outputs.py:79: assert message in messages
On 2019/04/12 20:17:12, rhowell wrote:
> NIT: This test might be a bit stricter if we used something like
> `set(expected_messages) == messages`, but it seems there's two extra broken
> links in there (that might've been my fault). Would it make sense to clean up
> those broken links? It's these two:
> 
> Link to "rel_path.html" (from "rel_path") cannot be resolved
> Link to "myscripts.js" (from "rel_path") cannot be resolved

Yes, I thought about it too. In the end I decided to not do anything about it:
- For the purposes of this test we're just checking that broken links get
reported properly, so if there are some other messages that we don't expect,
that's fine.
- I haven't  looked too deep into it but I assume `rel_path` needs those links
for testing path rewriting -- I thought I'd rather not meddle there without a
good reason.

[rhowell, 2019-04-16 01:46:59]

LGTM. :)

https://codereview.adblockplus.org/30044555/diff/30044559/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/30044555/diff/30044559/tests/test_page_out...
tests/test_page_outputs.py:79: assert message in messages
On 2019/04/15 16:48:45, Vasily Kuznetsov wrote:
> On 2019/04/12 20:17:12, rhowell wrote:
> > NIT: This test might be a bit stricter if we used something like
> > `set(expected_messages) == messages`, but it seems there's two extra broken
> > links in there (that might've been my fault). Would it make sense to clean
up
> > those broken links? It's these two:
> > 
> > Link to "rel_path.html" (from "rel_path") cannot be resolved
> > Link to "myscripts.js" (from "rel_path") cannot be resolved
> 
> Yes, I thought about it too. In the end I decided to not do anything about it:
> - For the purposes of this test we're just checking that broken links get
> reported properly, so if there are some other messages that we don't expect,
> that's fine.
> - I haven't  looked too deep into it but I assume `rel_path` needs those links
> for testing path rewriting -- I thought I'd rather not meddle there without a
> good reason.

Makes sense.