hacking check for jsonutils produces pep8 traceback

[Corey Wright]

the new jsonutils hacking check produces a pep8 traceback because it returns a set (column offset and error text) instead of an iterable (as logical line checks, like this check, should).

commit 243879f5c51fc45f03491bcb78765945ddf76be8
Change-Id: I86ed6cd3316dd4da5e1b10b36a3ddba3739316d3

===== 8< ===== TEST CASE ===== 8< =====
$ echo 'foo = json.dumps(bar)' >nova/foobar.py
$ flake8 -vv nova/foobar.py
local configuration: in /home/dev/Desktop/nova-test
  ignore = E121,E122,E123,E124,E125,E126,E127,E128,E129,E131,E251,H405,H803,H904
  exclude = .venv,.git,.tox,dist,doc,*openstack/common*,*lib/python*,*egg,build,tools
checking nova/foobar.py
foo = json.dumps(bar)
Traceback (most recent call last):
  File "/home/dev/Desktop/nova-test/.venv/bin/flake8", line 9, in <module>
    load_entry_point('flake8==2.1.0', 'console_scripts', 'flake8')()
  File "/home/dev/Desktop/nova-test/.venv/local/lib/python2.7/site-packages/flake8/main.py", line 32, in main
    report = flake8_style.check_files()
  File "/home/dev/Desktop/nova-test/.venv/local/lib/python2.7/site-packages/pep8.py", line 1672, in check_files
    runner(path)
  File "/home/dev/Desktop/nova-test/.venv/local/lib/python2.7/site-packages/flake8/engine.py", line 73, in input_file
    return fchecker.check_all(expected=expected, line_offset=line_offset)
  File "/home/dev/Desktop/nova-test/.venv/local/lib/python2.7/site-packages/pep8.py", line 1436, in check_all
    self.check_logical()
  File "/home/dev/Desktop/nova-test/.venv/local/lib/python2.7/site-packages/pep8.py", line 1338, in check_logical
    for offset, text in self.run_check(check, argument_names) or ():
TypeError: 'int' object is not iterable
===== 8< ===== TEST CASE ===== 8< =====

diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py
index a1dd614..7fe7412 100644
--- a/nova/hacking/checks.py
+++ b/nova/hacking/checks.py
@@ -300,7 +300,7 @@ def use_jsonutils(logical_line, filename):
         for f in json_funcs:
             pos = logical_line.find('json.%s' % f)
             if pos != -1:
- return (pos, msg % {'fun': f})
+ yield (pos, msg % {'fun': f})

 def factory(register):
===== 8< ===== PATCH ===== 8< =====

it's late, so tomorrow, if there hasn't been any activity on this, then i'll submit a patch for review.

[Corey Wright (coreywright)]

flake8 output after patch applied:

===== 8< ===== OUTPUT ===== 8< =====
$ echo 'foo = json.dumps(bar)' >nova/foobar.py
$ flake8 nova/foobar.py
nova/foobar.py:1:7: F821 undefined name 'json'
nova/foobar.py:1:7: N323 jsonutils.dump must be used instead of json.dump
nova/foobar.py:1:7: N323 jsonutils.dumps must be used instead of json.dumps
nova/foobar.py:1:18: F821 undefined name 'bar'
===== 8< ===== OUTPUT ===== 8< =====

ignore the F821 errors as they are a product of this merely being a single line example (ie i don't import the json module nor define the variable bar) and nothing more.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/114125

[Ihar Hrachyshka (ihar-hrachyshka)]

I agree that it should return iterable, though I don't think we should yield. I would better return 1-item list, so that we don't get multiple violation messages, as in:

nova/foobar.py:1:7: N323 jsonutils.dump must be used instead of json.dump
nova/foobar.py:1:7: N323 jsonutils.dumps must be used instead of json.dumps

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/114160

[OpenStack Infra (hudson-openstack)]

Change abandoned by Ihar Hrachyshka (<email address hidden>) on branch: master
Review: https://review.openstack.org/114160
Reason: I've realized it's already on review as: https://review.openstack.org/#/c/114125

[Corey Wright (coreywright)]

@Ihar

yeah, i'm an idiot (or maybe i shouldn't file bug reports late at night ;), but yes the fact that "dumps" is matched by both "dump" and "dumps" is unfortunate (and clearly visible in my example output, whihc i totally overlooked; wow, i really am an idiot / was tired last night).

@dims

i disagree with the importance (and i understand we might just have to agree to disagree and will hopefully be inconsequential because this should be resolved quickly) because of the unintuitiveness of the failure. when i originally encountered this bug during a "./run_test.sh --pep8" i had no inclination of what failed and it took me longer than desired to figure out the problem was with use_jsonutils and not until i familiarized myself with flake8's verbosity command-line options. if flake8 failed more gracefully, like "plugin use_jsonutils failed on file nova/foobar.py, line 1" then i would agree (because that's enough context that i could figure it out nearly immediately).

[Ihar Hrachyshka (ihar-hrachyshka)]

@Corey, there is actually a way to yield and still avoid incorrect violation errors (see the patch proposed by Joe).

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/114125
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=42d017c0d498aa4034032104f9cdd56300c866e0
Submitter: Jenkins
Branch: master

commit 42d017c0d498aa4034032104f9cdd56300c866e0
Author: Joe Gordon <email address hidden>
Date: Wed Aug 13 22:34:39 2014 -0700

    Fix hacking check for jsonutils

    Hacking checks with logical_lines should use yield not return.

    The jsonutils rule was added in:
    I86ed6cd3316dd4da5e1b10b36a3ddba3739316d3

    Now that the function uses yield, it can trigger twice in dumps (dump
    and dump), so look for a '(' at the afterwards. Don't display the '(' at
    the end of the error message since its confusing to read.

    Change-Id: I277dba08fdd30734409eee36008cebda35886968
    Closes-Bug: #1356687