Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(138)

Issue 29805580: Issue 6728 - Removes Mercurial dependency

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 days, 10 hours ago by Jon Sonesen
Modified:
2 days, 16 hours ago
Reviewers:
Vasily Kuznetsov
CC:
Sebastian Noack, juliandoucette, rhowell, mathias
Visibility:
Public.

Description

Issue 6728 - Removes Mercurial dependency

Patch Set 1 #

Patch Set 2 : Addremove files to diff #

Total comments: 7

Patch Set 3 : Address PS2 comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -126 lines) Patch
M cms/sources.py View 1 2 3 chunks +6 lines, -57 lines 0 comments Download
M tests/conftest.py View 1 chunk +0 lines, -8 lines 0 comments Download
A tests/expected_output/en/rel_path@static View 1 1 chunk +3 lines, -0 lines 0 comments Download
R tests/expected_output/en/rel_path@static+master View 1 1 chunk +0 lines, -3 lines 0 comments Download
M tests/expected_output/en/sitemap View 1 chunk +0 lines, -1 line 0 comments Download
R tests/expected_output/en/sitemap@static+master View 1 1 chunk +0 lines, -30 lines 0 comments Download
M tests/test_page_outputs.py View 1 2 2 chunks +12 lines, -26 lines 0 comments Download
M tox.ini View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 5
Jon Sonesen
5 days, 10 hours ago (2018-06-12 22:33:39 UTC) #1
mathias
https://codereview.adblockplus.org/29805580/diff/29805586/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29805580/diff/29805586/cms/sources.py#newcode238 cms/sources.py:238: return randint(1, 10) I think this should be a ...
5 days, 3 hours ago (2018-06-13 06:04:50 UTC) #2
Vasily Kuznetsov
Hi Jon, Looks pretty good, love the deleted code :D I'm thinking that perhaps we ...
4 days, 19 hours ago (2018-06-13 13:35:24 UTC) #3
Jon Sonesen
On 2018/06/13 13:35:24, Vasily Kuznetsov wrote: > Hi Jon, > > Looks pretty good, love ...
3 days, 13 hours ago (2018-06-14 19:24:07 UTC) #4
Jon Sonesen
3 days, 13 hours ago (2018-06-14 19:24:15 UTC) #5
https://codereview.adblockplus.org/29805580/diff/29805586/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29805580/diff/29805586/cms/sources.py#newc...
cms/sources.py:238: return randint(1, 10)
On 2018/06/13 13:35:23, Vasily Kuznetsov wrote:
> On 2018/06/13 06:04:50, mathias wrote:
> > I think this should be a way higher range. The likeliness of hitting the
same
> > number in two consecutive builds is probably too high otherwise.
> 
> Yeah, a range of 10 is too small. This would give us 10% chance of an
accidental
> collision. Something like a longint range (2^32) would be better.

My bad, meant to increase the size here, it was just a quick placeholder for
testing

https://codereview.adblockplus.org/29805580/diff/29805586/tests/conftest.py
File tests/conftest.py (left):

https://codereview.adblockplus.org/29805580/diff/29805586/tests/conftest.py#o...
tests/conftest.py:16: subprocess.check_call(['hg', 'init', site_dir])
On 2018/06/13 13:35:23, Vasily Kuznetsov wrote:
> Woohoo! All of this stuff GONE. Could even delete the copying if we could
> arrange for the cache to be stored in /tmp or something like this (just an
idea,
> doesn't have to be in this review).

Yeah that's a good idea, I reckon we should do this in a different commit/issue

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

https://codereview.adblockplus.org/29805580/diff/29805586/tests/test_page_out...
tests/test_page_outputs.py:58: return
On 2018/06/13 13:35:23, Vasily Kuznetsov wrote:
> Maybe this could just be replaced with `else:`.

Heh, yeah for sure whoops
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5