[Telemetry] Set dummy variable regex in tools/telemetry and tools/perf

[Telemetry] Set dummy variable regex in tools/telemetry and tools/perf

This change sets Pylint's dummy-variables-rgx variable so that variable names
consisting entirely of underscores will not trigger unused argument warnings.

BUG=475714
R=dtu,aiolos,nednguyen@google.com

Committed: https://crrev.com/0ee5d1c3e7b926381f4662222cefbb4224d66d63
Cr-Commit-Position: refs/heads/master@{#357130}

[eakuefner, 2015-05-04 21:53:32]

Please, take a look.

[dtu, 2015-05-04 22:19:18]

https://codereview.chromium.org/1114343003/diff/1/tools/perf/pylintrc
File tools/perf/pylintrc (right):

https://codereview.chromium.org/1114343003/diff/1/tools/perf/pylintrc#newcode16
tools/perf/pylintrc:16: dummy-variables-rgx=_+
I think these need to be in a section named [VARIABLES] ?

[eakuefner, 2015-05-04 22:21:14]

https://codereview.chromium.org/1114343003/diff/1/tools/perf/pylintrc
File tools/perf/pylintrc (right):

https://codereview.chromium.org/1114343003/diff/1/tools/perf/pylintrc#newcode16
tools/perf/pylintrc:16: dummy-variables-rgx=_+
On 2015/05/04 22:19:18, dtu wrote:
> I think these need to be in a section named [VARIABLES] ?

Done.

[dtu, 2015-05-04 22:26:39]

lgtm % nit

https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc
File tools/perf/pylintrc (right):

https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc#new...
tools/perf/pylintrc:16: # A regular expression matching the name of dummy
variables (i.e. expectedly
Ok, sorry, maybe it's better to not copy the standard message, but we should
explain why we're modifying this.

[eakuefner, 2015-05-04 22:40:03]

https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc
File tools/perf/pylintrc (right):

https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc#new...
tools/perf/pylintrc:16: # A regular expression matching the name of dummy
variables (i.e. expectedly
On 2015/05/04 22:26:39, dtu wrote:
> Ok, sorry, maybe it's better to not copy the standard message, but we should
> explain why we're modifying this.

Done.

[eakuefner, 2015-05-04 22:40:06]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2015-05-04 22:40:06]

The patchset sent to the CQ was uploaded after l-g-t-m from dtu@chromium.org
Link to the patchset: https://codereview.chromium.org/1114343003/#ps40001
(title: "Address Dave's second comment")

[commit-bot: I haz the power, 2015-05-04 22:42:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1114343003/40001

[nednguyen, 2015-05-04 22:48:14]

On 2015/05/04 22:26:39, dtu wrote:
> lgtm % nit
> 
> https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc
> File tools/perf/pylintrc (right):
> 
>
https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc#new...
> tools/perf/pylintrc:16: # A regular expression matching the name of dummy
> variables (i.e. expectedly
> Ok, sorry, maybe it's better to not copy the standard message, but we should
> explain why we're modifying this.

I actually prefer this work around:

def Foo(_1, _2, _3):
  del _1, _2, _3  # unused

Since:
1) This makes it much clearer to the readers that the variable is unused. 
2) In case someone modifies the method, and mistakenly makes use of the
variables, it will crash the program. A guaranteed crash is good in this case
because the callers may not expect the unused params to be used, and may pass
wrong values.
3) This may reduce the memory footprint. 
4) You usually need unused params when you want to override a method.

Class SuperClass:
  def foo(a, b, c):
    pass

Class DerivedClass(SuperClass):
  def foo (_a, _b, _c): 
    pass

callsite may write: 

x = DerivedClass()
x.foo(1, 2, c=3)  # crash

So it's better to keep the variable naming of derived method consistent with the
super, and use del in these cases.

[nednguyen, 2015-05-04 22:48:21]

The CQ bit was unchecked by nednguyen@google.com

[eakuefner, 2015-05-05 16:48:07]

On 2015/05/04 at 22:48:14, nednguyen wrote:
> On 2015/05/04 22:26:39, dtu wrote:
> > lgtm % nit
> > 
> > https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc
> > File tools/perf/pylintrc (right):
> > 
> >
https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc#new...
> > tools/perf/pylintrc:16: # A regular expression matching the name of dummy
> > variables (i.e. expectedly
> > Ok, sorry, maybe it's better to not copy the standard message, but we should
> > explain why we're modifying this.
> 
> I actually prefer this work around:
> 
> def Foo(_1, _2, _3):
>   del _1, _2, _3  # unused
> 
> Since:
> 1) This makes it much clearer to the readers that the variable is unused. 
> 2) In case someone modifies the method, and mistakenly makes use of the
variables, it will crash the program. A guaranteed crash is good in this case
because the callers may not expect the unused params to be used, and may pass
wrong values.
> 3) This may reduce the memory footprint. 
> 4) You usually need unused params when you want to override a method.
> 
> Class SuperClass:
>   def foo(a, b, c):
>     pass
> 
> Class DerivedClass(SuperClass):
>   def foo (_a, _b, _c): 
>     pass
> 
> callsite may write: 
> 
> x = DerivedClass()
> x.foo(1, 2, c=3)  # crash
> 
> So it's better to keep the variable naming of derived method consistent with
the super, and use del in these cases.

I still don't think I agree -- I don't think that littering the code with dels
is better than sticking to the common convention of '_' means "don't care".
Dave, do you have any feeling about this? I think my response to 1 and 2 is that
this is a common Python idiom, and even if it weren't, the overhead of
introducing the convention to people new to Telemetry would be at least as great
as making sure that they know about this del convention.

To 3, I almost always feel like playing garbage collector is a hasty
microoptimization, and in this case in particular I don't see how the memory
footprint is reduced in the aggregate because locals are cleaned up at the end
of the function anyway.

I'm not sure I understand the example in 4, but if this were a case where you
were expecting people to use keyword arguments, couldn't you just write def
foo(a, b, **_) in the derived class?

[eakuefner, 2015-05-05 17:02:48]

On 2015/05/05 at 16:48:07, eakuefner wrote:
> On 2015/05/04 at 22:48:14, nednguyen wrote:
> > On 2015/05/04 22:26:39, dtu wrote:
> > > lgtm % nit
> > > 
> > > https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc
> > > File tools/perf/pylintrc (right):
> > > 
> > >
https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc#new...
> > > tools/perf/pylintrc:16: # A regular expression matching the name of dummy
> > > variables (i.e. expectedly
> > > Ok, sorry, maybe it's better to not copy the standard message, but we
should
> > > explain why we're modifying this.
> > 
> > I actually prefer this work around:
> > 
> > def Foo(_1, _2, _3):
> >   del _1, _2, _3  # unused
> > 
> > Since:
> > 1) This makes it much clearer to the readers that the variable is unused. 
> > 2) In case someone modifies the method, and mistakenly makes use of the
variables, it will crash the program. A guaranteed crash is good in this case
because the callers may not expect the unused params to be used, and may pass
wrong values.
> > 3) This may reduce the memory footprint. 
> > 4) You usually need unused params when you want to override a method.
> > 
> > Class SuperClass:
> >   def foo(a, b, c):
> >     pass
> > 
> > Class DerivedClass(SuperClass):
> >   def foo (_a, _b, _c): 
> >     pass
> > 
> > callsite may write: 
> > 
> > x = DerivedClass()
> > x.foo(1, 2, c=3)  # crash
> > 
> > So it's better to keep the variable naming of derived method consistent with
the super, and use del in these cases.
> 
> I still don't think I agree -- I don't think that littering the code with dels
is better than sticking to the common convention of '_' means "don't care".
Dave, do you have any feeling about this? I think my response to 1 and 2 is that
this is a common Python idiom, and even if it weren't, the overhead of
introducing the convention to people new to Telemetry would be at least as great
as making sure that they know about this del convention.
> 
> To 3, I almost always feel like playing garbage collector is a hasty
microoptimization, and in this case in particular I don't see how the memory
footprint is reduced in the aggregate because locals are cleaned up at the end
of the function anyway.
> 
> I'm not sure I understand the example in 4, but if this were a case where you
were expecting people to use keyword arguments, couldn't you just write def
foo(a, b, **_) in the derived class?

Also, as a followup, going back to our benchmark_smoke_unittest example of
load_tests from the other CL, I realized that */** let you write def
load_tests(*_), which is arguably cleaner than def(_, __, ___).

[nednguyen, 2015-05-05 18:02:07]

On 2015/05/05 17:02:48, eakuefner wrote:
> On 2015/05/05 at 16:48:07, eakuefner wrote:
> > On 2015/05/04 at 22:48:14, nednguyen wrote:
> > > On 2015/05/04 22:26:39, dtu wrote:
> > > > lgtm % nit
> > > > 
> > > >
https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc
> > > > File tools/perf/pylintrc (right):
> > > > 
> > > >
>
https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc#new...
> > > > tools/perf/pylintrc:16: # A regular expression matching the name of
dummy
> > > > variables (i.e. expectedly
> > > > Ok, sorry, maybe it's better to not copy the standard message, but we
> should
> > > > explain why we're modifying this.
> > > 
> > > I actually prefer this work around:
> > > 
> > > def Foo(_1, _2, _3):
> > >   del _1, _2, _3  # unused
> > > 
> > > Since:
> > > 1) This makes it much clearer to the readers that the variable is unused. 
> > > 2) In case someone modifies the method, and mistakenly makes use of the
> variables, it will crash the program. A guaranteed crash is good in this case
> because the callers may not expect the unused params to be used, and may pass
> wrong values.
> > > 3) This may reduce the memory footprint. 
> > > 4) You usually need unused params when you want to override a method.
> > > 
> > > Class SuperClass:
> > >   def foo(a, b, c):
> > >     pass
> > > 
> > > Class DerivedClass(SuperClass):
> > >   def foo (_a, _b, _c): 
> > >     pass
> > > 
> > > callsite may write: 
> > > 
> > > x = DerivedClass()
> > > x.foo(1, 2, c=3)  # crash
> > > 
> > > So it's better to keep the variable naming of derived method consistent
with
> the super, and use del in these cases.
> > 
> > I still don't think I agree -- I don't think that littering the code with
dels
> is better than sticking to the common convention of '_' means "don't care".

Why is that littering the code? How much better is 
def foo(_a, _b, _c)

compared with 
def foo(a, b, c):
   del a, b, c  # unused
?

> Dave, do you have any feeling about this? I think my response to 1 and 2 is
that
> this is a common Python idiom, and even if it weren't, the overhead of
> introducing the convention to people new to Telemetry would be at least as
great
> as making sure that they know about this del convention.

That's why we have the code review process, isn't it?

> > 
> > To 3, I almost always feel like playing garbage collector is a hasty
> microoptimization, and in this case in particular I don't see how the memory
> footprint is reduced in the aggregate because locals are cleaned up at the end
> of the function anyway.
> > 
> > I'm not sure I understand the example in 4, but if this were a case where
you
> were expecting people to use keyword arguments, couldn't you just write def
> foo(a, b, **_) in the derived class?

What about the case a & c are used and b is not used in foo(a, b, c)?

> 
> Also, as a followup, going back to our benchmark_smoke_unittest example of
> load_tests from the other CL, I realized that */** let you write def
> load_tests(*_), which is arguably cleaner than def(_, __, ___).

[dtu, 2015-05-05 18:09:22]

On 2015/05/05 18:02:07, nednguyen (ooo til 05-07) wrote:
> On 2015/05/05 17:02:48, eakuefner wrote:
> > On 2015/05/05 at 16:48:07, eakuefner wrote:
> > > On 2015/05/04 at 22:48:14, nednguyen wrote:
> > > > On 2015/05/04 22:26:39, dtu wrote:
> > > > > lgtm % nit
> > > > > 
> > > > >
> https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc
> > > > > File tools/perf/pylintrc (right):
> > > > > 
> > > > >
> >
>
https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc#new...
> > > > > tools/perf/pylintrc:16: # A regular expression matching the name of
> dummy
> > > > > variables (i.e. expectedly
> > > > > Ok, sorry, maybe it's better to not copy the standard message, but we
> > should
> > > > > explain why we're modifying this.
> > > > 
> > > > I actually prefer this work around:
> > > > 
> > > > def Foo(_1, _2, _3):
> > > >   del _1, _2, _3  # unused
> > > > 
> > > > Since:
> > > > 1) This makes it much clearer to the readers that the variable is
unused. 
> > > > 2) In case someone modifies the method, and mistakenly makes use of the
> > variables, it will crash the program. A guaranteed crash is good in this
case
> > because the callers may not expect the unused params to be used, and may
pass
> > wrong values.
> > > > 3) This may reduce the memory footprint. 
> > > > 4) You usually need unused params when you want to override a method.
> > > > 
> > > > Class SuperClass:
> > > >   def foo(a, b, c):
> > > >     pass
> > > > 
> > > > Class DerivedClass(SuperClass):
> > > >   def foo (_a, _b, _c): 
> > > >     pass
> > > > 
> > > > callsite may write: 
> > > > 
> > > > x = DerivedClass()
> > > > x.foo(1, 2, c=3)  # crash
> > > > 
> > > > So it's better to keep the variable naming of derived method consistent
> with
> > the super, and use del in these cases.
> > > 
> > > I still don't think I agree -- I don't think that littering the code with
> dels
> > is better than sticking to the common convention of '_' means "don't care".
> 
> Why is that littering the code? How much better is 
> def foo(_a, _b, _c)
> 
> compared with 
> def foo(a, b, c):
>    del a, b, c  # unused
> ?
> 
> > Dave, do you have any feeling about this? I think my response to 1 and 2 is
> that
> > this is a common Python idiom, and even if it weren't, the overhead of
> > introducing the convention to people new to Telemetry would be at least as
> great
> > as making sure that they know about this del convention.
> 
> That's why we have the code review process, isn't it?
> 
> > > 
> > > To 3, I almost always feel like playing garbage collector is a hasty
> > microoptimization, and in this case in particular I don't see how the memory
> > footprint is reduced in the aggregate because locals are cleaned up at the
end
> > of the function anyway.
> > > 
> > > I'm not sure I understand the example in 4, but if this were a case where
> you
> > were expecting people to use keyword arguments, couldn't you just write def
> > foo(a, b, **_) in the derived class?
> 
> What about the case a & c are used and b is not used in foo(a, b, c)?
> 
> > 
> > Also, as a followup, going back to our benchmark_smoke_unittest example of
> > load_tests from the other CL, I realized that */** let you write def
> > load_tests(*_), which is arguably cleaner than def(_, __, ___).


I feel like the del pattern is pretty non-idiomatic. And relying on code review
to enforce it doesn't scale. Using _ as a name is common globally, and a very
clear indicator that the variable is unused.

No opinion on *_ vs _, __, ___. I think we use both in different places. The
example above would be foo(_, b, __)

[nednguyen(REVIEW IN OTHER ACC), 2015-05-05 22:10:21]

On 2015/05/05 18:09:22, dtu wrote:
> On 2015/05/05 18:02:07, nednguyen (ooo til 05-07) wrote:
> > On 2015/05/05 17:02:48, eakuefner wrote:
> > > On 2015/05/05 at 16:48:07, eakuefner wrote:
> > > > On 2015/05/04 at 22:48:14, nednguyen wrote:
> > > > > On 2015/05/04 22:26:39, dtu wrote:
> > > > > > lgtm % nit
> > > > > > 
> > > > > >
> > https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc
> > > > > > File tools/perf/pylintrc (right):
> > > > > > 
> > > > > >
> > >
> >
>
https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc#new...
> > > > > > tools/perf/pylintrc:16: # A regular expression matching the name of
> > dummy
> > > > > > variables (i.e. expectedly
> > > > > > Ok, sorry, maybe it's better to not copy the standard message, but
we
> > > should
> > > > > > explain why we're modifying this.
> > > > > 
> > > > > I actually prefer this work around:
> > > > > 
> > > > > def Foo(_1, _2, _3):
> > > > >   del _1, _2, _3  # unused
> > > > > 
> > > > > Since:
> > > > > 1) This makes it much clearer to the readers that the variable is
> unused. 
> > > > > 2) In case someone modifies the method, and mistakenly makes use of
the
> > > variables, it will crash the program. A guaranteed crash is good in this
> case
> > > because the callers may not expect the unused params to be used, and may
> pass
> > > wrong values.
> > > > > 3) This may reduce the memory footprint. 
> > > > > 4) You usually need unused params when you want to override a method.
> > > > > 
> > > > > Class SuperClass:
> > > > >   def foo(a, b, c):
> > > > >     pass
> > > > > 
> > > > > Class DerivedClass(SuperClass):
> > > > >   def foo (_a, _b, _c): 
> > > > >     pass
> > > > > 
> > > > > callsite may write: 
> > > > > 
> > > > > x = DerivedClass()
> > > > > x.foo(1, 2, c=3)  # crash
> > > > > 
> > > > > So it's better to keep the variable naming of derived method
consistent
> > with
> > > the super, and use del in these cases.
> > > > 
> > > > I still don't think I agree -- I don't think that littering the code
with
> > dels
> > > is better than sticking to the common convention of '_' means "don't
care".
> > 
> > Why is that littering the code? How much better is 
> > def foo(_a, _b, _c)
> > 
> > compared with 
> > def foo(a, b, c):
> >    del a, b, c  # unused
> > ?
> > 
> > > Dave, do you have any feeling about this? I think my response to 1 and 2
is
> > that
> > > this is a common Python idiom, and even if it weren't, the overhead of
> > > introducing the convention to people new to Telemetry would be at least as
> > great
> > > as making sure that they know about this del convention.
> > 
> > That's why we have the code review process, isn't it?
> > 
> > > > 
> > > > To 3, I almost always feel like playing garbage collector is a hasty
> > > microoptimization, and in this case in particular I don't see how the
memory
> > > footprint is reduced in the aggregate because locals are cleaned up at the
> end
> > > of the function anyway.
> > > > 
> > > > I'm not sure I understand the example in 4, but if this were a case
where
> > you
> > > were expecting people to use keyword arguments, couldn't you just write
def
> > > foo(a, b, **_) in the derived class?
> > 
> > What about the case a & c are used and b is not used in foo(a, b, c)?
> > 
> > > 
> > > Also, as a followup, going back to our benchmark_smoke_unittest example of
> > > load_tests from the other CL, I realized that */** let you write def
> > > load_tests(*_), which is arguably cleaner than def(_, __, ___).
> 
> 
> I feel like the del pattern is pretty non-idiomatic. And relying on code
review
> to enforce it doesn't scale. 
> Using _ as a name is common globally, and a very
> clear indicator that the variable is unused.
I agree that _ prefix is the conventional way of declaring not-to-use variables,
but between following idiom and less erroneous code, I choose the latter. If
code review doea not scale, we still have pylint and good documentation to
explain why del is preferred over using underscore in the case of function
params.
> No opinion on *_ vs _, __, ___. I think we use both in different places. The
> example above would be foo(_, b, __)

This will create bug when the call site does objectA.foo(a=1, b=2, c=3). As
stated above, it's best to keep the params name the same as the superclass's
method, especially when we use discovery pattern in telemetry quite often for
dynamically picking classes and instantiate objects.

[eakuefner, 2015-05-05 22:30:01]

On 2015/05/05 at 22:10:21, nednguyen wrote:
> On 2015/05/05 18:09:22, dtu wrote:
> > On 2015/05/05 18:02:07, nednguyen (ooo til 05-07) wrote:
> > > On 2015/05/05 17:02:48, eakuefner wrote:
> > > > On 2015/05/05 at 16:48:07, eakuefner wrote:
> > > > > On 2015/05/04 at 22:48:14, nednguyen wrote:
> > > > > > On 2015/05/04 22:26:39, dtu wrote:
> > > > > > > lgtm % nit
> > > > > > > 
> > > > > > >
> > > https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc
> > > > > > > File tools/perf/pylintrc (right):
> > > > > > > 
> > > > > > >
> > > >
> > >
> >
https://codereview.chromium.org/1114343003/diff/20001/tools/perf/pylintrc#new...
> > > > > > > tools/perf/pylintrc:16: # A regular expression matching the name
of
> > > dummy
> > > > > > > variables (i.e. expectedly
> > > > > > > Ok, sorry, maybe it's better to not copy the standard message, but
we
> > > > should
> > > > > > > explain why we're modifying this.
> > > > > > 
> > > > > > I actually prefer this work around:
> > > > > > 
> > > > > > def Foo(_1, _2, _3):
> > > > > >   del _1, _2, _3  # unused
> > > > > > 
> > > > > > Since:
> > > > > > 1) This makes it much clearer to the readers that the variable is
> > unused. 
> > > > > > 2) In case someone modifies the method, and mistakenly makes use of
the
> > > > variables, it will crash the program. A guaranteed crash is good in this
> > case
> > > > because the callers may not expect the unused params to be used, and may
> > pass
> > > > wrong values.
> > > > > > 3) This may reduce the memory footprint. 
> > > > > > 4) You usually need unused params when you want to override a
method.
> > > > > > 
> > > > > > Class SuperClass:
> > > > > >   def foo(a, b, c):
> > > > > >     pass
> > > > > > 
> > > > > > Class DerivedClass(SuperClass):
> > > > > >   def foo (_a, _b, _c): 
> > > > > >     pass
> > > > > > 
> > > > > > callsite may write: 
> > > > > > 
> > > > > > x = DerivedClass()
> > > > > > x.foo(1, 2, c=3)  # crash
> > > > > > 
> > > > > > So it's better to keep the variable naming of derived method
consistent
> > > with
> > > > the super, and use del in these cases.
> > > > > 
> > > > > I still don't think I agree -- I don't think that littering the code
with
> > > dels
> > > > is better than sticking to the common convention of '_' means "don't
care".
> > > 
> > > Why is that littering the code? How much better is 
> > > def foo(_a, _b, _c)
> > > 
> > > compared with 
> > > def foo(a, b, c):
> > >    del a, b, c  # unused
> > > ?
> > > 
> > > > Dave, do you have any feeling about this? I think my response to 1 and 2
is
> > > that
> > > > this is a common Python idiom, and even if it weren't, the overhead of
> > > > introducing the convention to people new to Telemetry would be at least
as
> > > great
> > > > as making sure that they know about this del convention.
> > > 
> > > That's why we have the code review process, isn't it?
> > > 
> > > > > 
> > > > > To 3, I almost always feel like playing garbage collector is a hasty
> > > > microoptimization, and in this case in particular I don't see how the
memory
> > > > footprint is reduced in the aggregate because locals are cleaned up at
the
> > end
> > > > of the function anyway.
> > > > > 
> > > > > I'm not sure I understand the example in 4, but if this were a case
where
> > > you
> > > > were expecting people to use keyword arguments, couldn't you just write
def
> > > > foo(a, b, **_) in the derived class?
> > > 
> > > What about the case a & c are used and b is not used in foo(a, b, c)?
> > > 
> > > > 
> > > > Also, as a followup, going back to our benchmark_smoke_unittest example
of
> > > > load_tests from the other CL, I realized that */** let you write def
> > > > load_tests(*_), which is arguably cleaner than def(_, __, ___).
> > 
> > 
> > I feel like the del pattern is pretty non-idiomatic. And relying on code
review
> > to enforce it doesn't scale. 
> > Using _ as a name is common globally, and a very
> > clear indicator that the variable is unused.
> I agree that _ prefix is the conventional way of declaring not-to-use
variables, but between following idiom and less erroneous code, I choose the
latter. If code review doea not scale, we still have pylint and good
documentation to explain why del is preferred over using underscore in the case
of function params.
> > No opinion on *_ vs _, __, ___. I think we use both in different places. The
> > example above would be foo(_, b, __)
> 
> This will create bug when the call site does objectA.foo(a=1, b=2, c=3). As
stated above, it's best to keep the params name the same as the superclass's
method, especially when we use discovery pattern in telemetry quite often for
dynamically picking classes and instantiate objects.

Okay, enough people on SO seem to think that the del pattern is an okay idea
that I guess I'm okay with allowing it for the sake of allowing the linting to
be better for now.

I remain skeptical of allowing this for the long term since I think our two
major use cases here are 1. things like tests where the classes you write aren't
really used outside the file, so underscores would be fine and 2. things that
are discovered. We spoke a long time ago about broadly switching away from
positional arguments for things like the value system where discovery takes
place, and in this case, using kwargs dicts gives you the ability to handle or
ignore arguments as you please without using del.

I'm fine to revisit this once we're in a position to actually eradicate
positional callsites for discovered types.

[aiolos (Not reviewing), 2015-05-05 22:56:14]

From the style guide:

Unused argument warnings can be suppressed by using `_' as the identifier for
the unused argument or prefixing the argument name with `unused_'. In situations
where changing the argument names is infeasible, you can mention them at the
beginning of the function. For example:

def foo(a, unused_b, unused_c, d=None, e=None):
    _ = d, e
    return a


I don't think del should be explicitly disallowed, but it shouldn't be a
requirement for the codebase in general.

[eakuefner, 2015-05-05 23:06:39]

New patchset reflects styleguide.

On 2015/05/05 at 22:56:14, aiolos wrote:

> I don't think del should be explicitly disallowed, but it shouldn't be a
requirement for the codebase in general.
Agree.

[nednguyen, 2015-05-05 23:50:58]

On 2015/05/05 23:06:39, eakuefner wrote:
> New patchset reflects styleguide.
> 
> On 2015/05/05 at 22:56:14, aiolos wrote:
> 
> > I don't think del should be explicitly disallowed, but it shouldn't be a
> requirement for the codebase in general.
> Agree.

Eradicate positional call sites for discovered types is a much harder problem to
solve with code review. And even for case without discovered types, people will
still use the method based on the signature of the interface classes.

An example of how code review process can miss this: someone modifies the
MacPlatformBackend::ReadMsr(self, msr_number, start=0, length=64) and the
implementation doesn't use the "start" variable, hence they suppress pylint
check with ReadMsr(self, msr_number, _start=0, length=64). Assume that there is
some client code which already writes :
if platform_backend.SupportMsr():
   platform_backend.ReadMsr(msr_number=100, start=12, length=45)

Since client usage code does not show up in the patch, it's very tricky to
discover this bug unless there is some unittest that catches this. 

The problem without not enforcing the "del" standard in the codebase is 9 out of
10 cases, most people (including me) will have strong urge to use foo(_x, _y,
_z) instead of writting del x, y, z, and they miss the subtle reason of why it
can be buggy. The pylint wiki page has explicitly encourage the "del" pattern
and against changing parameter names:
http://pylint-messages.wikidot.com/messages:w0613

Instead, if we abandon the usage of _params everywhere in telemetry, enable
pylint to catch violations & add documentation in pylintrc explaining why we
don't adopt that pattern, I don't think being different from usual idioms matter
too much. Idioms are coding practices formed by the repeated use. If people
adopt "del" more due to stability reason more, it will become idioms. 

For Kari's style guide quote, I would also point out style guide values
consistency the most, and they aren't meant to be a replacement over local
style:

"The point of having style guidelines is to have a common vocabulary of coding
so people can concentrate on what you are saying rather than on how you are
saying it. We present global style rules here so people know the vocabulary, but
local style is also important. If code you add to a file looks drastically
different from the existing code around it, it throws readers out of their
rhythm when they go to read it. Try to avoid this."

(https://code.google.com/p/soc/wiki/PythonStyleGuide#Conclusion)

[aiolos (Not reviewing), 2015-05-06 00:23:10]

On 2015/05/05 23:50:58, nednguyen (ooo til 05-07) wrote:
> On 2015/05/05 23:06:39, eakuefner wrote:
> > New patchset reflects styleguide.
> > 
> > On 2015/05/05 at 22:56:14, aiolos wrote:
> > 
> > > I don't think del should be explicitly disallowed, but it shouldn't be a
> > requirement for the codebase in general.
> > Agree.
> 
> Eradicate positional call sites for discovered types is a much harder problem
to
> solve with code review. And even for case without discovered types, people
will
> still use the method based on the signature of the interface classes.
> 
> An example of how code review process can miss this: someone modifies the
> MacPlatformBackend::ReadMsr(self, msr_number, start=0, length=64) and the
> implementation doesn't use the "start" variable, hence they suppress pylint
> check with ReadMsr(self, msr_number, _start=0, length=64). Assume that there
is
> some client code which already writes :
> if platform_backend.SupportMsr():
>    platform_backend.ReadMsr(msr_number=100, start=12, length=45)
> 
> Since client usage code does not show up in the patch, it's very tricky to
> discover this bug unless there is some unittest that catches this. 
> 
> The problem without not enforcing the "del" standard in the codebase is 9 out
of
> 10 cases, most people (including me) will have strong urge to use foo(_x, _y,
> _z) instead of writting del x, y, z, and they miss the subtle reason of why it
> can be buggy. The pylint wiki page has explicitly encourage the "del" pattern
> and against changing parameter names:
> http://pylint-messages.wikidot.com/messages:w0613
> 
> Instead, if we abandon the usage of _params everywhere in telemetry, enable
> pylint to catch violations & add documentation in pylintrc explaining why we
> don't adopt that pattern, I don't think being different from usual idioms
matter
> too much. Idioms are coding practices formed by the repeated use. If people
> adopt "del" more due to stability reason more, it will become idioms. 

I think you could make a case that del is a better coding practice. If that is
the point you are trying to make, I would even agree with you. But that doesn't
change the fact that it is inconsistent with current coding practices in our
codebase, and three of the four people in our office are unconvinced that the
gains we would get from changing over to using it aren't worth the added hassle
of making it a requirement. And of the three of us, I'm probably the most on
your side. I don't think that is going to change over codereview. If we want to
discuss this further, we should probably setup a meeting for it. But I wouldn't
count on being able to change our minds.

> 
> For Kari's style guide quote, I would also point out style guide values
> consistency the most, and they aren't meant to be a replacement over local
> style:
> 
> "The point of having style guidelines is to have a common vocabulary of coding
> so people can concentrate on what you are saying rather than on how you are
> saying it. We present global style rules here so people know the vocabulary,
but
> local style is also important. If code you add to a file looks drastically
> different from the existing code around it, it throws readers out of their
> rhythm when they go to read it. Try to avoid this."
> 
> (https://code.google.com/p/soc/wiki/PythonStyleGuide#Conclusion)


The style guide values consistency with existing code. This quote is actually an
argument against us using del, even if we changed the style guide to recommend
it, because we don't already use it.

Additionally, I vehemently disagree that we should be purposefully allowing new
local style that is inconsistent with the style guide. There is no reason to
have a style guide if you are just going to ignore it, and I have very little
interest in working in a codebase that does so.

[nednguyen, 2015-05-06 00:38:29]

On 2015/05/06 00:23:10, aiolos wrote:
> On 2015/05/05 23:50:58, nednguyen (ooo til 05-07) wrote:
> > On 2015/05/05 23:06:39, eakuefner wrote:
> > > New patchset reflects styleguide.
> > > 
> > > On 2015/05/05 at 22:56:14, aiolos wrote:
> > > 
> > > > I don't think del should be explicitly disallowed, but it shouldn't be a
> > > requirement for the codebase in general.
> > > Agree.
> > 
> > Eradicate positional call sites for discovered types is a much harder
problem
> to
> > solve with code review. And even for case without discovered types, people
> will
> > still use the method based on the signature of the interface classes.
> > 
> > An example of how code review process can miss this: someone modifies the
> > MacPlatformBackend::ReadMsr(self, msr_number, start=0, length=64) and the
> > implementation doesn't use the "start" variable, hence they suppress pylint
> > check with ReadMsr(self, msr_number, _start=0, length=64). Assume that there
> is
> > some client code which already writes :
> > if platform_backend.SupportMsr():
> >    platform_backend.ReadMsr(msr_number=100, start=12, length=45)
> > 
> > Since client usage code does not show up in the patch, it's very tricky to
> > discover this bug unless there is some unittest that catches this. 
> > 
> > The problem without not enforcing the "del" standard in the codebase is 9
out
> of
> > 10 cases, most people (including me) will have strong urge to use foo(_x,
_y,
> > _z) instead of writting del x, y, z, and they miss the subtle reason of why
it
> > can be buggy. The pylint wiki page has explicitly encourage the "del"
pattern
> > and against changing parameter names:
> > http://pylint-messages.wikidot.com/messages:w0613
> > 
> > Instead, if we abandon the usage of _params everywhere in telemetry, enable
> > pylint to catch violations & add documentation in pylintrc explaining why we
> > don't adopt that pattern, I don't think being different from usual idioms
> matter
> > too much. Idioms are coding practices formed by the repeated use. If people
> > adopt "del" more due to stability reason more, it will become idioms. 
> 
> I think you could make a case that del is a better coding practice. If that is
> the point you are trying to make, I would even agree with you. But that
doesn't
> change the fact that it is inconsistent with current coding practices in our
> codebase, 

I realize this, and we should switch the existing usage to use "del ..."

> and three of the four people in our office are unconvinced that the
> gains we would get from changing over to using it aren't worth the added
hassle
> of making it a requirement. And of the three of us, I'm probably the most on
> your side. I don't think that is going to change over codereview. If we want
to
> discuss this further, we should probably setup a meeting for it. But I
wouldn't
> count on being able to change our minds.
> 
> > 
> > For Kari's style guide quote, I would also point out style guide values
> > consistency the most, and they aren't meant to be a replacement over local
> > style:
> > 
> > "The point of having style guidelines is to have a common vocabulary of
coding
> > so people can concentrate on what you are saying rather than on how you are
> > saying it. We present global style rules here so people know the vocabulary,
> but
> > local style is also important. If code you add to a file looks drastically
> > different from the existing code around it, it throws readers out of their
> > rhythm when they go to read it. Try to avoid this."
> > 
> > (https://code.google.com/p/soc/wiki/PythonStyleGuide#Conclusion)
> 
> 
> The style guide values consistency with existing code. This quote is actually
an
> argument against us using del, even if we changed the style guide to recommend
> it, because we don't already use it.
> 
> Additionally, I vehemently disagree that we should be purposefully allowing
new
> local style that is inconsistent with the style guide. There is no reason to
> have a style guide if you are just going to ignore it, and I have very little
> interest in working in a codebase that does so.

I agree that both extreme of "always follow style guide" & "abandon style guide"
should be avoid at all cost. However, in this particular case, we purposefully
make a new local style, because it's a better programming practice that makes
program less erroneous, not because I think the code looks prettier (on the
contrary, I already stated that I would lean over to using the _ prefix if both
are allowed). And my proposal is that we switch all existing code to use this
"del" pattern for consistency & improving code health. 

I will be more than happy to discuss about this in person when I'm back in the
office.

[nednguyen, 2015-05-06 02:21:35]

On 2015/05/06 00:38:29, nednguyen (ooo til 05-07) wrote:
> On 2015/05/06 00:23:10, aiolos wrote:
> > On 2015/05/05 23:50:58, nednguyen (ooo til 05-07) wrote:
> > > On 2015/05/05 23:06:39, eakuefner wrote:
> > > > New patchset reflects styleguide.
> > > > 
> > > > On 2015/05/05 at 22:56:14, aiolos wrote:
> > > > 
> > > > > I don't think del should be explicitly disallowed, but it shouldn't be
a
> > > > requirement for the codebase in general.
> > > > Agree.
> > > 
> > > Eradicate positional call sites for discovered types is a much harder
> problem
> > to
> > > solve with code review. And even for case without discovered types, people
> > will
> > > still use the method based on the signature of the interface classes.
> > > 
> > > An example of how code review process can miss this: someone modifies the
> > > MacPlatformBackend::ReadMsr(self, msr_number, start=0, length=64) and the
> > > implementation doesn't use the "start" variable, hence they suppress
pylint
> > > check with ReadMsr(self, msr_number, _start=0, length=64). Assume that
there
> > is
> > > some client code which already writes :
> > > if platform_backend.SupportMsr():
> > >    platform_backend.ReadMsr(msr_number=100, start=12, length=45)
> > > 
> > > Since client usage code does not show up in the patch, it's very tricky to
> > > discover this bug unless there is some unittest that catches this. 
> > > 
> > > The problem without not enforcing the "del" standard in the codebase is 9
> out
> > of
> > > 10 cases, most people (including me) will have strong urge to use foo(_x,
> _y,
> > > _z) instead of writting del x, y, z, and they miss the subtle reason of
why
> it
> > > can be buggy. The pylint wiki page has explicitly encourage the "del"
> pattern
> > > and against changing parameter names:
> > > http://pylint-messages.wikidot.com/messages:w0613
> > > 
> > > Instead, if we abandon the usage of _params everywhere in telemetry,
enable
> > > pylint to catch violations & add documentation in pylintrc explaining why
we
> > > don't adopt that pattern, I don't think being different from usual idioms
> > matter
> > > too much. Idioms are coding practices formed by the repeated use. If
people
> > > adopt "del" more due to stability reason more, it will become idioms. 
> > 
> > I think you could make a case that del is a better coding practice. If that
is
> > the point you are trying to make, I would even agree with you. But that
> doesn't
> > change the fact that it is inconsistent with current coding practices in our
> > codebase, 
> 
> I realize this, and we should switch the existing usage to use "del ..."
> 
> > and three of the four people in our office are unconvinced that the
> > gains we would get from changing over to using it aren't worth the added
> hassle
> > of making it a requirement. And of the three of us, I'm probably the most on
> > your side. I don't think that is going to change over codereview. If we want
> to
> > discuss this further, we should probably setup a meeting for it. But I
> wouldn't
> > count on being able to change our minds.
> > 
> > > 
> > > For Kari's style guide quote, I would also point out style guide values
> > > consistency the most, and they aren't meant to be a replacement over local
> > > style:
> > > 
> > > "The point of having style guidelines is to have a common vocabulary of
> coding
> > > so people can concentrate on what you are saying rather than on how you
are
> > > saying it. We present global style rules here so people know the
vocabulary,
> > but
> > > local style is also important. If code you add to a file looks drastically
> > > different from the existing code around it, it throws readers out of their
> > > rhythm when they go to read it. Try to avoid this."
> > > 
> > > (https://code.google.com/p/soc/wiki/PythonStyleGuide#Conclusion)
> > 
> > 
> > The style guide values consistency with existing code. This quote is
actually
> an
> > argument against us using del, even if we changed the style guide to
recommend
> > it, because we don't already use it.
> > 
> > Additionally, I vehemently disagree that we should be purposefully allowing
> new
> > local style that is inconsistent with the style guide. There is no reason to
> > have a style guide if you are just going to ignore it, and I have very
little
> > interest in working in a codebase that does so.
> 
> I agree that both extreme of "always follow style guide" & "abandon style
guide"
> should be avoid at all cost. However, in this particular case, we purposefully
> make a new local style, because it's a better programming practice that makes
> program less erroneous, not because I think the code looks prettier (on the
> contrary, I already stated that I would lean over to using the _ prefix if
both
> are allowed). And my proposal is that we switch all existing code to use this
> "del" pattern for consistency & improving code health. 
> 
> I will be more than happy to discuss about this in person when I'm back in the
> office.

Actually, I underestimate the number of the existing usage of foo(_x, _y)
pattern in telemetry/ , and we shouldn't block the "shrinking pylintrc" work,
which is more important overall goal, due to this. So I agree Kari's opinion of
allowing both. 

lgtm

[nednguyen, 2015-05-06 02:25:38]

https://codereview.chromium.org/1114343003/diff/60001/tools/perf/pylintrc
File tools/perf/pylintrc (right):

https://codereview.chromium.org/1114343003/diff/60001/tools/perf/pylintrc#new...
tools/perf/pylintrc:17: dummy-variables-rgx=_$|unused_
We don't have any of "unused_" usage in telemetry, so I think
"dummy-variables-rgx=_" with documentation in patch #3 is good enough?

[eakuefner, 2015-05-06 02:28:51]

On 2015/05/06 at 02:25:38, nednguyen wrote:
> https://codereview.chromium.org/1114343003/diff/60001/tools/perf/pylintrc
> File tools/perf/pylintrc (right):
> 
>
https://codereview.chromium.org/1114343003/diff/60001/tools/perf/pylintrc#new...
> tools/perf/pylintrc:17: dummy-variables-rgx=_$|unused_
> We don't have any of "unused_" usage in telemetry, so I think
"dummy-variables-rgx=_" with documentation in patch #3 is good enough?

I'm in favor of allowing unused_.* for the simple purpose of not deviating from
the style guide in ways that aren't sufficiently motivated. If we allow dummy
variables, I think _.* is not materially different from _|unused_.*, so as the
latter conforms to style guide, it gets my vote.

[eakuefner, 2015-05-06 02:30:37]

On 2015/05/06 at 02:28:51, eakuefner wrote:
> On 2015/05/06 at 02:25:38, nednguyen wrote:
> > https://codereview.chromium.org/1114343003/diff/60001/tools/perf/pylintrc
> > File tools/perf/pylintrc (right):
> > 
> >
https://codereview.chromium.org/1114343003/diff/60001/tools/perf/pylintrc#new...
> > tools/perf/pylintrc:17: dummy-variables-rgx=_$|unused_
> > We don't have any of "unused_" usage in telemetry, so I think
"dummy-variables-rgx=_" with documentation in patch #3 is good enough?
> 
> I'm in favor of allowing unused_.* for the simple purpose of not deviating
from the style guide in ways that aren't sufficiently motivated. If we allow
dummy variables, I think _.* is not materially different from _|unused_.*, so as
the latter conforms to style guide, it gets my vote.

(Also, I'd be willing to change any _, __, ___ to match Google style as part of
this change, based on this)

[eakuefner, 2015-05-06 03:31:38]

The CQ bit was checked by eakuefner@chromium.org to run a CQ dry run

[eakuefner, 2015-05-06 03:31:39]

The patchset sent to the CQ was uploaded after l-g-t-m from dtu@chromium.org
Link to the patchset: https://codereview.chromium.org/1114343003/#ps60001
(title: "Address Kari's comment")

[commit-bot: I haz the power, 2015-05-06 03:31:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1114343003/60001

[commit-bot: I haz the power, 2015-05-06 04:34:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-06 04:34:15]

Dry run: This issue passed the CQ dry run.

[nednguyen, 2015-05-06 11:34:59]

On 2015/05/06 02:30:37, eakuefner wrote:
> On 2015/05/06 at 02:28:51, eakuefner wrote:
> > On 2015/05/06 at 02:25:38, nednguyen wrote:
> > > https://codereview.chromium.org/1114343003/diff/60001/tools/perf/pylintrc
> > > File tools/perf/pylintrc (right):
> > > 
> > >
>
https://codereview.chromium.org/1114343003/diff/60001/tools/perf/pylintrc#new...
> > > tools/perf/pylintrc:17: dummy-variables-rgx=_$|unused_
> > > We don't have any of "unused_" usage in telemetry, so I think
> "dummy-variables-rgx=_" with documentation in patch #3 is good enough?
> > 
> > I'm in favor of allowing unused_.* for the simple purpose of not deviating
> from the style guide in ways that aren't sufficiently motivated. If we allow
> dummy variables, I think _.* is not materially different from _|unused_.*, so
as
> the latter conforms to style guide, it gets my vote.
> 
> (Also, I'd be willing to change any _, __, ___ to match Google style as part
of
> this change, based on this)

sgtm. I think you may want to make some edit in telemetry/telemetry &
tools/perf/.. so it triggers the linter. Otherwise, the next person who make
changes to telemetry will encounter a huge lint error.

[aiolos (Not reviewing), 2015-10-29 23:57:06]

ping.

[dtu, 2015-10-30 07:33:30]

lgtm

[nednguyen, 2015-10-30 15:32:36]

On 2015/10/30 07:33:30, dtu wrote:
> lgtm

Ethan: please make some change to telemetry/ & perf/ code before landing this to
make sure that this does not create extra PRESUBMIT error for the existing code.
I remember there was a time I made some pylint change and it blocked all the
subsequent telemetry/ & perf/ patches because pylint runs on those directories
wasn't triggered with the pylintrc change.

[aiolos (Not reviewing), 2015-10-30 15:50:51]

On 2015/10/30 15:32:36, nednguyen wrote:
> On 2015/10/30 07:33:30, dtu wrote:
> > lgtm
> 
> Ethan: please make some change to telemetry/ & perf/ code before landing this
to
> make sure that this does not create extra PRESUBMIT error for the existing
code.
> I remember there was a time I made some pylint change and it blocked all the
> subsequent telemetry/ & perf/ patches because pylint runs on those directories
> wasn't triggered with the pylintrc change.

+1 I also suggest rebasing.

lgtm

[eakuefner, 2015-10-30 16:54:01]

I have verified both that the regex works as expected, and that this change
won't cause any lint blowups. I fixed one unused-variable warning in Telemetry
as a result of the new pylintrc.

It turns out that dummy-variable-rgx governs both unused-argument and
unused-variable warnings, and unused-argument is still disabled in Telemetry. We
have over 100 unused-argument warnings in Telemetry, so I've filed
crbug.com/549651 about re-enabling unused-argument. There you'll find a
spreadsheet that we can use to track the progress of re-enabling.

[eakuefner, 2015-10-30 16:54:06]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2015-10-30 16:54:06]

The patchset sent to the CQ was uploaded after l-g-t-m from aiolos@chromium.org,
nednguyen@google.com, dtu@chromium.org
Link to the patchset: https://codereview.chromium.org/1114343003/#ps80001
(title: "freshen CL")

[commit-bot: I haz the power, 2015-10-30 16:54:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1114343003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1114343003/80001

[commit-bot: I haz the power, 2015-10-30 18:26:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-30 18:27:06]

Patchset 5 (id:??) landed as
https://crrev.com/0ee5d1c3e7b926381f4662222cefbb4224d66d63
Cr-Commit-Position: refs/heads/master@{#357130}