Issue 6545 - get_pages_metadata now returns all pages

Issue 6545 - get_pages_metadata now returns all pages

[Jon Sonesen, 2018-04-16 20:18:07]

Hey guys,

Added both of you to the review as there are significant changes to test suite
design and the `:` character is reserved on windows, however I don't believe
this package would work on windows as is anyway but is worth noting.

Cheers!

[Jon Sonesen, 2018-04-16 20:23:00]

Actually, my bad I got the test suite changes mixed up with a different issue
(5618) which will require bookmark specific files my bad!

[Jon Sonesen, 2018-04-16 20:54:12]

On 2018/04/16 20:23:00, Jon Sonesen wrote:
> Actually, my bad I got the test suite changes mixed up with a different issue
> (5618) which will require bookmark specific files my bad!

I will fix this bad patch sets and reupload

[Jon Sonesen, 2018-04-17 00:01:34]

On 2018/04/16 20:54:12, Jon Sonesen wrote:
> On 2018/04/16 20:23:00, Jon Sonesen wrote:
> > Actually, my bad I got the test suite changes mixed up with a different
issue
> > (5618) which will require bookmark specific files my bad!
> 
> I will fix this bad patch sets and reupload

Okay, yeah I fixed it the way I was calling one of the function was quietly
skipping some tests which is why the first patch set was passing despite being
wrong. 

These changes to the test suite could probably use commenting or adding to the
documentation which I am happy to do, but perhaps we should discuss it first,
the idea is that now we can add revision specific version of expected output
pages. The only use case right now is the sitemap global which now returns
'en/bar' when the bookmark is None, but perhaps this will change in the future
and I don't think having the flexibility to do so in the future is such a bad
idea.

[Vasily Kuznetsov, 2018-04-17 18:36:08]

Hi Jon,

Looks good in general. Hg is complaining that ':' is a reserved symbol on
Windows so I guess we'd better come up with something else, otherwise the tests
will not work on Windows. Perhaps '+' -- I tried and it seems to work.

Also see my comment below.

Cheers,
Vasily

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

https://codereview.adblockplus.org/29753617/diff/29753652/tests/test_page_out...
tests/test_page_outputs.py:20: if ':' in filename:
It seems like the code in this `if` is not necessary. The filename will either
have '@' + test_type in it or not. If it does, the assignment of line 22 has
been already done by line 19. If it doesn't, realname will be the same as
filename and then line 22 is a noop.

Or am I confused?

[Jon Sonesen, 2018-04-17 21:02:40]

Thanks for taking a look

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

https://codereview.adblockplus.org/29753617/diff/29753652/tests/test_page_out...
tests/test_page_outputs.py:20: if ':' in filename:
On 2018/04/17 18:36:08, Vasily Kuznetsov wrote:
> It seems like the code in this `if` is not necessary. The filename will either
> have '@' + test_type in it or not. If it does, the assignment of line 22 has
> been already done by line 19. If it doesn't, realname will be the same as
> filename and then line 22 is a noop.
> 
> Or am I confused?

As discussed very IRC the code is necessary however, the reason tests pass is
that there is now edge cases in the static test function that will now quietly
pass tests that don't fit the correct formatting. I will change that, thanks for
your comment!

[Jon Sonesen, 2018-04-26 01:12:40]

On 2018/04/17 21:02:40, Jon Sonesen wrote:
> Thanks for taking a look
> 
>
https://codereview.adblockplus.org/29753617/diff/29753652/tests/test_page_out...
> File tests/test_page_outputs.py (right):
> 
>
https://codereview.adblockplus.org/29753617/diff/29753652/tests/test_page_out...
> tests/test_page_outputs.py:20: if ':' in filename:
> On 2018/04/17 18:36:08, Vasily Kuznetsov wrote:
> > It seems like the code in this `if` is not necessary. The filename will
either
> > have '@' + test_type in it or not. If it does, the assignment of line 22 has
> > been already done by line 19. If it doesn't, realname will be the same as
> > filename and then line 22 is a noop.
> > 
> > Or am I confused?
> 
> As discussed very IRC the code is necessary however, the reason tests pass is
> that there is now edge cases in the static test function that will now quietly
> pass tests that don't fit the correct formatting. I will change that, thanks
for
> your comment!

Ok, so now there are two tickets, one for the documentation and one for this
issue (thanks Vasily)

[Vasily Kuznetsov, 2018-05-03 15:11:19]

LGTM (although needs to be updated a bit to land on the current master, but
that's simple).

[Jon Sonesen, 2018-05-06 23:00:22]

On 2018/05/03 15:11:19, Vasily Kuznetsov wrote:
> LGTM (although needs to be updated a bit to land on the current master, but
> that's simple).

Hey,


thanks, I just had an idea about updating the comment there so if you dont mind
taking a look that would be awesome!

[Vasily Kuznetsov, 2018-05-07 16:42:39]

On 2018/05/06 23:00:22, Jon Sonesen wrote:
> On 2018/05/03 15:11:19, Vasily Kuznetsov wrote:
> > LGTM (although needs to be updated a bit to land on the current master, but
> > that's simple).
> 
> Hey,
> 
> 
> thanks, I just had an idea about updating the comment there so if you dont
mind
> taking a look that would be awesome!

Even more LGTM!