Revert "pylint: upgrade to 1.3.1"

Revert "pylint: upgrade to 1.3.1"

This reverts commit e642ee868c9191c9ba90b0afa0fa2bceb31312bd.

Conflicts:
	pylintrc

Reason:
	All repos are borked.

TBR=vapier

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=293042

[smut, 2014-11-20 20:02:27]

smut@google.com changed reviewers:
+ vapier@chromium.org

[smut, 2014-11-20 20:02:56]

Committed patchset #1 (id:1) manually as 293042.

[vapier, 2014-11-20 20:21:17]

uhh, you need to provide real data.  "all repos are broke" provides absolutely
nothing useful.  we already updated the pylintrc config to disable the majority
of new warnings seen in a couple of repos.  no one has mentioned anything since.

[smut, 2014-11-20 20:29:05]

On 2014/11/20 20:21:17, vapier wrote:
> uhh, you need to provide real data.  "all repos are broke" provides absolutely
> nothing useful.  we already updated the pylintrc config to disable the
majority
> of new warnings seen in a couple of repos.  no one has mentioned anything
since.

You're in a thread about this:
https://groups.google.com/a/chromium.org/forum/#!topic/infra-dev/JCkhcY1TfgY.

[vapier, 2014-11-20 20:44:15]

and that thread was resolved by disabling the vast majority of new warnings.  so
please provide some concrete examples using the latest depot_tools.

[smut, 2014-11-20 21:08:49]

On 2014/11/20 20:44:15, vapier wrote:
> and that thread was resolved by disabling the vast majority of new warnings. 
so
> please provide some concrete examples using the latest depot_tools.

Read the thread again. Nothing is resolved, you just ignored everyone
recommending that the pylint upgrade be reverted and instead tried to fix it,
and unsuccessfully so. Here's a sample CL that was rejected by the commit queue
for unrelated lint errors: https://codereview.chromium.org/744913002. This was a
critical fix that needed to land, but was fucked by the pylint roll.

[vapier, 2014-11-20 21:47:20]

wow, if this revert didn't land, i'd think you were just screwing around to try
and be funny.  did you actually read that lint warning ?  that code was broken
because it was using a variable *that did not exist*.

and you know what ?  someone actually fixed it:
https://codereview.chromium.org/747453003

and you know what else ?  the old version of pylint *would have done the same
exact thing and rejected your commit*.

so if you're not actually following what's going on, please stop screwing around
with this code.

[smut, 2014-11-20 21:52:43]

On 2014/11/20 21:47:20, vapier wrote:
> wow, if this revert didn't land, i'd think you were just screwing around to
try
> and be funny.  did you actually read that lint warning ?  that code was broken
> because it was using a variable *that did not exist*.
> 
> and you know what ?  someone actually fixed it:
> https://codereview.chromium.org/747453003
> 
> and you know what else ?  the old version of pylint *would have done the same
> exact thing and rejected your commit*.
> 
> so if you're not actually following what's going on, please stop screwing
around
> with this code.

See, this is exactly the problem that people were talking about on the thread.
Nobody can tell what a legitimate pylint error is anymore because every repo has
dozens of brand new pylint errors. None of us trying to commit that CL noticed
that this was a legitimate presubmit breakage caused by another CL.

[vapier, 2014-11-20 21:56:00]

again, you need to provide *concrete examples* of warnings that shouldn't exist.
 like i already said, i disabled all the new ones i saw and were pointed out in
that thread.  providing output that *only shows legit errors* isn't a use case.

the other aspect was the volume.  in this case, you had literally one error to
read.  claiming one legitimate line was too much is incredulous.

[vapier, 2014-11-20 21:56:02]

again, you need to provide *concrete examples* of warnings that shouldn't exist.
 like i already said, i disabled all the new ones i saw and were pointed out in
that thread.  providing output that *only shows legit errors* isn't a use case.

the other aspect was the volume.  in this case, you had literally one error to
read.  claiming one legitimate line was too much is incredulous.

[tonyg, 2014-11-20 22:17:32]

Telemetry (~65kloc python) updated all of its lint warnings to the new syntax
already over several changes, the bulk of them in
https://codereview.chromium.org/739623005/. This was a temporary pain, but we
already paid the cost and are happy w/ the new linter. Then, this revert broke
our presubmit again because the old linter doesn't respect the new linter's
syntax. We're in the middle of a fixit week and all patches are now blocked.

I don't want to get in the middle of this, but we need to know immediately
whether we should roll back the lint fixes (which'll already cause merge pain
for us) or whether we're going to move forward with the new linter (which means
no additional work for us).

[vapier, 2014-11-20 22:19:27]

i've posted a CL to reland pylint 1.3.  i was waiting on a lgtm, but if it's
breaking people, i can push it through.

i'm not against fixing new problems, but all the real problems reported thus far
(such as all the ones listed in that infra-dev thread) are fixed.  vague
references isn't helping anyone.

[tonyg, 2014-11-20 22:22:53]

On 2014/11/20 22:19:27, vapier wrote:
> i've posted a CL to reland pylint 1.3.  i was waiting on a lgtm, but if it's
> breaking people, i can push it through.
> 
> i'm not against fixing new problems, but all the real problems reported thus
far
> (such as all the ones listed in that infra-dev thread) are fixed.  vague
> references isn't helping anyone.

I like the way the original change was announced so that we knew that we'd have
to do a little work to make the new linter happy. But the revert was a total
surprise and the code base is moving too fast to flap back and forth without
announcement like this patch did. If you're looking for an lgtm for the
re-enabling, send me a link and I'll stamp.

[ghost stip (do not use), 2014-11-20 22:29:50]

On 2014/11/20 21:47:20, vapier wrote:
> wow, if this revert didn't land, i'd think you were just screwing around to
try
> and be funny.  did you actually read that lint warning ?  that code was broken
> because it was using a variable *that did not exist*.
> 
> and you know what ?  someone actually fixed it:
> https://codereview.chromium.org/747453003
> 
> and you know what else ?  the old version of pylint *would have done the same
> exact thing and rejected your commit*.
> 
> so if you're not actually following what's going on, please stop screwing
around
> with this code.

"that code was broken because it was using a variable *that did not exist*."

incorrect, unrelated code in the repository triggered that warning. I'm glad the
drmemory bug got fixed, but this is why it would have been nice to DEPS in the
pylint change so projects can change when they're ready. please keep that in
mind next time you make extremely global changes like this.

[vapier, 2014-11-20 22:34:25]

i'm aware the bug in that module was unrelated to the CL in question, but the
infra peeps indicate that's WAI.  changes to one module might break others which
means they all need to be checked.

however, conflating broken code with a pylint upgrade is wrong -- please read my
other responses.  the old pylint would have done the same exact thing.

[ghost stip (do not use), 2014-11-20 22:38:11]

On 2014/11/20 22:34:25, vapier wrote:
> i'm aware the bug in that module was unrelated to the CL in question, but the
> infra peeps indicate that's WAI.  changes to one module might break others
which
> means they all need to be checked.
> 
> however, conflating broken code with a pylint upgrade is wrong -- please read
my
> other responses.  the old pylint would have done the same exact thing.

I see, the offending code was committed in
https://codereview.chromium.org/744833002 and it looks like the person did not
run presubmit.

+1 on the un-revert (reinstating new pylint).