jenkins flake8/pyflakes fails all patches due to new rules rejecting old code
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Xqt
	May 4 2016, 4:32 AM

Description

pyflakes 1.2.0 feature https://github.com/pyflakes/pyflakes/pull/59 rejects existing pywikibot-core code, causing all new gerrit changes to be rejected by Jenkins 's Python 3 lint rules until this is solved.

See https://integration.wikimedia.org/ci/job/tox-jessie/7611/console from https://gerrit.wikimedia.org/r/286691

04:27:53 ./scripts/flickrripper.py:318:35: F821 undefined name '_tk_error'
04:28:05 ./pywikibot/botirc.py:34:19: F821 undefined name 'e'
04:28:06 ./pywikibot/diff.py:584:15: F821 undefined name 'bserror'

Details

	Subject	Repo	Branch	Lines +/-
	pywikibot: Store ImportError in imported variable	pywikibot/core	2.0	+8 -13
	pywikibot: Store ImportError in imported variable	pywikibot/core	master	+7 -12

Customize query in gerrit

Related Objects
Search...

		Status	Subtype	Assigned	Task
		Resolved		AbdealiJK	T134336 jenkins flake8/pyflakes fails all patches due to new rules rejecting old code
		Resolved		AbdealiJK	T134341 Create tests for diff.html_comparator

Event Timeline

Xqt created this task.May 4 2016, 4:32 AM

Restricted Application added subscribers: pywikibot-bugs-list, Zppix, Aklapper. · View Herald TranscriptMay 4 2016, 4:32 AM

Xqt added subscribers: jayvdb, hashar.May 4 2016, 4:42 AM

jayvdb updated the task description. (Show Details)May 4 2016, 4:51 AM

I think this is caused by pyflakes: 1.2.0 , released today (see https://pypi.python.org/pypi/pyflakes, but 1.2.0 is missing from https://github.com/pyflakes/pyflakes/releases) , specifically https://github.com/pyflakes/pyflakes/pull/59 which I recommended against.

confirmed pypi 'checker.py' is master 'checker.py', so v1.2.0 includes that pull request

jayvdb merged a task: T134337: Use of invalid variable for ImportErrors.May 4 2016, 5:29 AM

jayvdb added a subscriber: AbdealiJK.

Oops, just uploaded a patch set for T134337
Should I change the commit message to use this task ?

@AbdealiJK , on T134337 , you wrote "The [current] method id actually invalid because python doesn't guarantee that the variable _tk_error exists outside the except statement."

That is the reason behind https://github.com/pyflakes/pyflakes/pull/59 .
./scripts/flickrripper.py and ./pywikibot/botirc.py are not effectively tested on Python 3, and are not a high priority.

However ./pywikibot/diff.py is a central component, and is used by the Python 3 tests, so it should be possible to create a test case that proves that the current use of bserror can cause an UnboundLocalError.

In T134336#2262507, @AbdealiJK wrote:

Oops, just uploaded a patch set for T134337
Should I change the commit message to use this task ?

Yes please.
But first I would like to confirm that this is a real bug, and not a false positive.

@jayvdb irrespective of whether this is a false positive, I think it should be changed in diff.py because of convention.

I remember when trying to do a similar thing in T123092 @Mpaa gave the gerrit comment:

This is more in line with implementation of the same kind in other modules.

Although I do agree it would be good to have a testcase for it. (I have no idea how to test it though !)

In T134336#2262524, @AbdealiJK wrote:

@jayvdb irrespective of whether this is a false positive, I think it should be changed in diff.py because of convention.

I remember when trying to do a similar thing in T123092 @Mpaa gave the gerrit comment:

This is more in line with implementation of the same kind in other modules.

Although I do agree it would be good to have a testcase for it. (I have no idea how to test it though !)

Yes, I'm 100% supporting the changes you've made, which standardise our approach to runtime detection of external packages.

In T134336#2262509, @jayvdb wrote:

...
However ./pywikibot/diff.py is a central component, and is used by the Python 3 tests, so it should be possible to create a test case that proves that the current use of bserror can cause an UnboundLocalError.

Ah, the only use of bserror is of by html_comparator, which is not used anywhere and thus not covered by the tests.
(and IMO we shouldnt be loading BeautifulSoup at the top of the diff module if it will never be used; it is slowing down startup)

(and not surprising, I suggested it be placed into diff.py while reviewing https://gerrit.wikimedia.org/r/#/c/182561/ PS5)

Change 286787 had a related patch set uploaded (by AbdealiJK):
pywikibot: Store ImportError in imported variable

https://gerrit.wikimedia.org/r/286787

gerritbot added a project: Patch-For-Review.May 4 2016, 6:07 AM

jayvdb created subtask T134341: Create tests for diff.html_comparator.May 4 2016, 6:16 AM

Change 286787 merged by jenkins-bot:
pywikibot: Store ImportError in imported variable

https://gerrit.wikimedia.org/r/286787

jayvdb closed this task as Resolved.May 4 2016, 7:26 AM

jayvdb assigned this task to AbdealiJK.

AbdealiJK closed subtask T134341: Create tests for diff.html_comparator as Resolved.May 15 2016, 11:15 PM

jayvdb renamed this task from flake8-py3 fails for unknown reason to jenkins flake8/pyflakes fails all patches due to new rules rejecting old code.May 16 2016, 2:14 AM

jayvdb removed a subtask: T134341: Create tests for diff.html_comparator.

jayvdb added a subtask: T134341: Create tests for diff.html_comparator.

jayvdb updated the task description. (Show Details)May 16 2016, 2:17 AM

jayvdb updated the task description. (Show Details)

Change 296411 had a related patch set uploaded (by John Vandenberg):
pywikibot: Store ImportError in imported variable

https://gerrit.wikimedia.org/r/296411

Change 296411 merged by jenkins-bot:
pywikibot: Store ImportError in imported variable