Extend PEP8 coverage

[stvstnfrd]

It looks like we weren't running PEP8 against the entire repo.
This changeset addresses this by:

• parametering the list of directories to be linted [1]
• adding additional directories to the list [2]
• fixing the newly discovered PEP8 violations

This could be reviewed commit-by-commit, though it's probably small
enough to process in a single pass.

Let me know if you have any questions; thanks!

[1] The list was hard-coded to cover:

• common
• cms
• lms

[2] The new items include:

• docs
• openedx
• pavelib
• scripts

[openedx-webhooks]

Thanks for the pull request, @stvstnfrd! I've created OSPR-407 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• edx-code email threads
• timeline information ('this must be merged by XX date', and why that is)
• partner information ('this is a course on edx.org')
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here.

[stvstnfrd]

@sarina

No rush on this, just paying down some debt; whenever you have the time.

[sarina]

@stvstnfrd looks like you missed one: https://build.testeng.edx.org/job/edx-platform-all-tests-pr/1690/SHARD=1,TEST_SUITE=quality,label_exp=jenkins-worker/artifact/reports/pavelib/pep8.report/*view*/

[sarina]

👍 from me. Lets get a passing build and I also want someone from test (maybe @benpatterson) to take a look,

[sarina]

@stvstnfrd : I think the quality build may still be failing because there's an increased # of Pylint violations. You may need to ratchet the violation limit up temporarily so we can get this in.

Although... not sure about that. Your build report claims there's only 6134 and the limit is supposed to be 6300. However I can't figure out why else the quality shard is failing.

[sarina]

Also I wonder if we should get rid of the # nopep8 pragmas that are around the codebase. AFAIK this doesn't actually disable pep8, it just makes certain editors ignore the violations.

[benpatterson]

@stvstnfrd @sarina I looked over the failed build and I don't understand this exception:

03:29:50 Traceback (most recent call last):
03:29:50   File "/home/jenkins/edx-venv/local/lib/python2.7/site-packages/paver/tasks.py", line 195, in _run_task
03:29:50     return do_task()
03:29:50   File "/home/jenkins/edx-venv/local/lib/python2.7/site-packages/paver/tasks.py", line 192, in do_task
03:29:50     return func(**kw)
03:29:50   File "pavelib/quality.py", line 91, in run_pylint
03:29:50     dirs = os.listdir(os.path.join(system, directory))
03:29:50 OSError: [Errno 2] No such file or directory: 'openedx/lib'
03:29:50 

Perhaps that's why it failed?

[sarina]

@stvstnfrd it looks like whaty ou've got in quality.py is

for system in systems:
    ...
    for directory in ['lib']:
        ...

but now that you've extended systems to include more dirs than common,lms,cms, you need to add code that accounts for the fact that not all the new dirs have a 'lib' directory. I would recommend running the paver quality command locally since other bugs may arise.

[stvstnfrd]

@sarina Yeah, I noticed that earlier; haven't had a chance to fully test yet.

I may not get to this tomorrow, but I should be able to knock it out this weekend.

[stvstnfrd]

@sarina The build is now passing :)

[stvstnfrd]

I'm not going to include it in this PR, but I wanted to start a
conversation about the use of diff-quality in a violation-free world.
Of course, we still need diff-quality for Pylint, but now that this
repo is free of PEP8 violoations, can we stop running it for PEP8 and
instead just invoke PEP8 directly?

An issue I've run into is that I'll often get a different list of
violations depending on how I invoke the quality check; pep8 can yield
different results than diff-quality --pep8 (sic) or even paver run_pep8. This is presumably due to the command-line args and/or config
file used in each case.

TL;DR: Can paver run_quality be simplified to just run pep8 against
the entire repo, removing the diff-quality logic since commits must
have 0 PEP8 violations?

@sarina @benpatterson ^

[sarina]

Is there a reason to hardcode lib & djangoapps rather than using the ALL_SUBSYSTEMS variable? Also in the get_path_list function you make sure that these subdirs exist - is that a concern/convention we should follow here?

[stvstnfrd]

Is there a reason to hardcode lib & djangoapps rather than using the ALL_SUBSYSTEMS variable?

No reason; parameterized in 747756a

Also in the get_path_list function you make sure that these subdirs exist - is that a concern/convention we should follow here?

There isn't a concern here; this is used to set the PYTHONPATH environment variable and it doesn't matter if the directories don't exist. They are silently ignored; I tested this locally.

[sarina]

TL;DR: Can paver run_quality be simplified to just run pep8 against
the entire repo, removing the diff-quality logic since commits must
have 0 PEP8 violations?

I think that's a reasonable idea. It would be nice if we could make it so that there is a similar pep8 report that is attached to the diff-quality-pylint report in jenkins so there's no change in where you find the reports on jenkins. I think that should be done in a different PR.

[sarina]

👍 from me. I'm going to put in the test engineering backlog for @benpatterson or another test engineer to review the changes to how pep8 is calculated and consider the diff-quality question.

[stvstnfrd]

Also I wonder if we should get rid of the # nopep8 pragmas that are around the codebase. AFAIK this doesn't actually disable pep8, it just makes certain editors ignore the violations.

Removed in 4131898

[stvstnfrd]

I recommend rebasing as I think new pep8 violations have been added to the openedx directory.

Rebased on HEAD at 0c1cc07

[stvstnfrd]

@sarina I believe I've incorporated all requests. Please review at your leisure and let me know if I've overlooked anything.

[sarina]

@stvstnfrd looks good to me! Will wait to see what the build says...

[stvstnfrd]

@sarina Build says

[sarina]

/me bows down to the mighty Ahnold

[benpatterson]

Unfortunately, looks like a bug was introduced with this change, which results in pylint going unreported for the common dir. See the size of common/pylint.report here (new) vs here (old)

Here's what's passed into the terminal the old way:

PYTHONPATH=common:common/djangoapps:common/lib:common/djangoapps:common/lib pylint  --msg-template="{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}" common calc capa chem dogstats i18n safe_lxml sandbox-packages symmath xmodule | tee /edx/app/edxapp/edx-platform/reports/common/pylint.report

..and the new way (which only looks at common/__init__.py)

PYTHONPATH=common/lib:openedx/lib:openedx/djangoapps:common:common/djangoapps pylint  --msg-template="{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}" common | tee /edx/app/edxapp/edx-platform/reports/common/pylint.report

You'll notice that only the common dir is called by pylint the new way, and the old way, most of the subdirs under common/lib are called. (Perhaps all...I've run out of time...)

Unfortunately I don't have a ton of time for triaging this today. I'll plan to revert this afternoon unless someone can jump in to solve it.... Thx for any help!

[sarina]

@benpatterson I'm in meetings all day today - I think we should just revert this for now, and discuss with Steven what's up when he's online.

[benpatterson]

@stvstnfrd @andy-armstrong I reverted only the pylint refactoring commits.

IIRC Andy was working on making sure pylint works against the new dirs, so potentially that just clears the air for him?

Anyway I'd like to include those dirs...let me know how I can help from here.

[andy-armstrong]

@stvstnfrd @sarina @benpatterson is there a plan for getting the PyLint checking back for openedx? @cahrens just noticed it again in #7327.

[benpatterson]

@andy-armstrong thanks for bumping the thread.

Sounds like perhaps this was not on yours or @stvstnfrd 's plates? Sorry for the confusion; I thought it had been. I can certainly help move this along if nothing was moving for either of you guys.

[sarina]

@benpatterson would the simpler approach of #7114 work? Maybe we should go w/ that?

[benpatterson]

Works for me, @sarina . I just forget if that would restrict it to openedx/lib.

[stvstnfrd]

Hey @sarina @benpatterson @andy-armstrong sorry to have dropped the ball on this one. I could revisit this over the weekend, but if you're looking to move more quickly, please don't let me hold you back. Either way, let me know.

[benpatterson]

@stvstnfrd if you look at this over the weekend, that'd be great. I know on my end, I've got a few quarter-end things I need to take care of, and this would be a side-of-the-desk effort.