Hub 10953 - Anonymize IPs in access logs

[tlucas, 2018-05-08 16:08:22]

Hey Lena!

Just a few nits (out of interest, do you use flake8 with our extension?).

Best,
Tristan

https://codereview.adblockplus.org/29774565/diff/29774566/anonymize.py
File anonymize.py (right):

https://codereview.adblockplus.org/29774565/diff/29774566/anonymize.py#newcode70
anonymize.py:70: description='Filter out sensitive data from access logs',
What do you think about
...
        description=__doc__, 
        formatter_class=argparse.RawDescriptionHelpFormatter,
...

That way you'd have all that well-written module documentation as the output for


$ python anonymize.py -h

https://codereview.adblockplus.org/29774565/diff/29774566/anonymize.py#newcode85
anonymize.py:85: help=('Path to MaxMind DB file with GeoLite2 Country data,
defaults '
Those parentheses are redundant (and trigger our custom A111 in flake8)

https://codereview.adblockplus.org/29774565/diff/29774566/anonymize.py#newcode96
anonymize.py:96: 
Nit: this blank line is redundant.

[l.kryvonos, 2018-05-08 16:26:19]

I don't; I wasn't aware we had an extension for flake8.
The speed of the review definitely deserves that beer :)

https://codereview.adblockplus.org/29774565/diff/29774566/anonymize.py
File anonymize.py (right):

https://codereview.adblockplus.org/29774565/diff/29774566/anonymize.py#newcode70
anonymize.py:70: description='Filter out sensitive data from access logs',
On 2018/05/08 16:08:21, tlucas wrote:
> What do you think about
> ...
>         description=__doc__, 
>         formatter_class=argparse.RawDescriptionHelpFormatter,
> ...
> 
> That way you'd have all that well-written module documentation as the output
for
> 
> 
> $ python anonymize.py -h

I like doing `description=__doc__`, but I think the `__doc__` in this case is
too detailed. To my knowledge, this script will be used in between two pipes,
and not invoked manually (in which scenario exploring `-h` comes up often). And
if it fails and someone needs to understand where's the problem, they'll open
the code anyway.
What do you think?

https://codereview.adblockplus.org/29774565/diff/29774566/anonymize.py#newcode85
anonymize.py:85: help=('Path to MaxMind DB file with GeoLite2 Country data,
defaults '
On 2018/05/08 16:08:21, tlucas wrote:
> Those parentheses are redundant (and trigger our custom A111 in flake8)  

Acknowledged.

https://codereview.adblockplus.org/29774565/diff/29774566/anonymize.py#newcode96
anonymize.py:96: 
On 2018/05/08 16:08:21, tlucas wrote:
> Nit: this blank line is redundant.

Do all of our repositories follow the 'no blank line at the end of file'
convention?

[tlucas, 2018-05-08 16:40:56]

On 2018/05/08 16:26:19, l.kryvonos wrote:
> I don't; I wasn't aware we had an extension for flake8.
> The speed of the review definitely deserves that beer :)
> 
It's here -> https://hg.adblockplus.org/codingtools/file/tip/flake8-eyeo

More over, for a full suite of commonly used flake8-extensions you can refer to
some of our tox.ini files, e.g.
https://hg.adblockplus.org/buildtools/file/tip/tox.ini )

https://codereview.adblockplus.org/29774565/diff/29774566/anonymize.py
File anonymize.py (right):

https://codereview.adblockplus.org/29774565/diff/29774566/anonymize.py#newcode70
anonymize.py:70: description='Filter out sensitive data from access logs',
On 2018/05/08 16:26:19, l.kryvonos wrote:
> On 2018/05/08 16:08:21, tlucas wrote:
> > What do you think about
> > ...
> >         description=__doc__, 
> >         formatter_class=argparse.RawDescriptionHelpFormatter,
> > ...
> > 
> > That way you'd have all that well-written module documentation as the output
> for
> > 
> > 
> > $ python anonymize.py -h
> 
> I like doing `description=__doc__`, but I think the `__doc__` in this case is
> too detailed. To my knowledge, this script will be used in between two pipes,
> and not invoked manually (in which scenario exploring `-h` comes up often).
And
> if it fails and someone needs to understand where's the problem, they'll open
> the code anyway.
> What do you think?

Fair enough - let's see if those encountering the errors are keen enough to dig
through code ;)

https://codereview.adblockplus.org/29774565/diff/29774566/anonymize.py#newcode96
anonymize.py:96: 
On 2018/05/08 16:26:19, l.kryvonos wrote:
> On 2018/05/08 16:08:21, tlucas wrote:
> > Nit: this blank line is redundant.
> 
> Do all of our repositories follow the 'no blank line at the end of file'
> convention?

Yes - or at least they should (this is also a built-in flake8 warning; W391)

[tlucas, 2018-05-09 07:45:46]

On 2018/05/09 07:42:59, l.kryvonos wrote:

LGTM