Fix Pylint violations (easy) by stvstnfrd · Pull Request #7013 · openedx/openedx-platform

stvstnfrd · 2015-02-18T06:58:48Z

This is a subset to partially replace #7008.

@sarina You were right; piecemeal is the way to go here.

This set should be pretty easy to review; most of these are self-evident without context (the fun comes later).

openedx-webhooks · 2015-02-18T06:58:50Z

Thanks for the pull request, @stvstnfrd! I've created OSPR-412 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.

singingwolfboy · 2015-02-18T18:23:58Z

Are we certain that nothing relies on depth as a positional argument?

My extensive grepping reveals no matches.

I found this hard to believe at first, but looking at the function signature for other definitions of get_courses, (self, **kwargs) seems to actually match what we already have elsewhere.

singingwolfboy · 2015-02-18T18:27:23Z

👍

Also, it looks like the Jenkins build only failed because of the newly-exposed pylint violations, which were actually there the whole time. For example, empty docstrings are worse than no docstrings at all, but pylint didn't catch the empty docstrings. I think this PR is ready to merge, despite the Jenkins "failure".

stvstnfrd · 2015-02-18T20:36:24Z

@singingwolfboy Yeah, it looks like the only "failures" are from 4
diff-quality warnings (pylint) [1]:

3 are for missing-docstring
1 is for an invalid-name (too short)

Each of these issues was already present; they're just being exposed to
diff-quality because the lines are being updated.

The invalid-name I've already fixed in a separate branch. The
missing-docstring is an issue throughout our codebase; it's the
single largest violation, representing ~20% of all Pylint violations.

But to re-emphasize, these issues already existed in the code, so I'd
prefer to not rabbit-hole further for now; please advise.

[1] https://build.testeng.edx.org/job/edx-platform-all-tests-pr/1773/SHARD=1,TEST_SUITE=quality,label_exp=jenkins-worker/artifact/reports/diff_quality/diff_quality_pylint.html

sarina · 2015-02-18T20:43:33Z

@stvstnfrd generally I think I agree, that it's fine to merge this without a fully-passing build. Let me take a shot at reviewing this first.

sarina · 2015-02-18T20:44:54Z

@stvstnfrd this drops us to 6044 pylint violations. Personally I think it'd be best to adjust the PYLINT_THRESHOLD as we merge changes in, but if you disagree I'm open to a different approach.

stvstnfrd · 2015-02-18T20:45:50Z

@sarina I agree; allow me to tack on an additional commit to drop the threshold.

stvstnfrd · 2015-02-18T21:04:28Z

I have added an additional commit to lower the threshold at 0e997c7.

sarina · 2015-02-18T21:05:07Z

Can we move this comment to above L12, so we understand why we need the pragma?

durrrhh nevermind realized L12 is actually a removal not an addition n/m

sarina · 2015-02-19T13:51:18Z

👍 once last 2 comments addressed.

stvstnfrd · 2015-02-19T23:07:22Z

@sarina I have removed the useless test and added a TODO comment about fixing the other test later.

sarina · 2015-02-20T02:34:41Z

Huh - it doesn't seem like the build is actually running. I wonder if you're lacking some of the infrastructure to make our recent Jenkins upgrades run properly. I'm going to cancel the build, and if you could rebase this, we'll see if that gets the build going properly.

@sarina

Per @sarina: > `build_problem` is called in `_make_problem`, which other tests call > in this class. A failure here will arise in other tests. This test is > unnecessary.

sarina · 2015-02-20T14:34:25Z

I'm working with test eng to get the build stuff straightened out - it's not running properly, not your fault at all. Will merge as soon as I can get it all figured out.

sarina · 2015-02-20T17:21:19Z

test this please

stvstnfrd · 2015-02-20T17:45:53Z

No worries; let me know how I can help.

sarina · 2015-02-20T18:21:16Z

test this please

sarina · 2015-02-20T19:19:14Z

ok to test

sarina · 2015-02-20T21:35:48Z

Alright the failing tests on this branch are flaky and I believe are already disabled in master. Plus they passed on the lbuild I ran on my identical branch (#7049).

Fix Pylint violations (easy)

stvstnfrd · 2015-02-20T21:58:13Z

w00t! I'll try to assemble another set this weekend!

openedx-webhooks added the needs triage label Feb 18, 2015

singingwolfboy reviewed Feb 18, 2015
View reviewed changes

openedx-webhooks added community manager review and removed needs triage labels Feb 18, 2015

sarina reviewed Feb 18, 2015
View reviewed changes

stvstnfrd force-pushed the pylint/low branch from 0e997c7 to 9b7a69a Compare February 19, 2015 07:05

stvstnfrd added 11 commits February 19, 2015 19:56

Fix Pylint: C0326(bad-whitespace)

75aaf0d

Fix Pylint: C0112(empty-docstring)

416f709

Fix Pylint: W0613(unused-argument)

2e1400f

Fix Pylint: E0110(abstract-class-instantiated)

ee2404c

Fix Pylint: E0012(bad-option-value)

e17504d

Fix Pylint: I0022(deprecated-pragma)

66f28fd

Fix Pylint: C0202(bad-classmethod-argument)

49fb966

Fix Pylint: F0010(parse-error)

a43cd3f

Fix Pylint: E1102(not-callable)

c6ef359

Fix Pylint: W0107(unnecessary-pass)

69695a8

Fix Pylint: W1503(redundant-unittest-assert)

c0a7bf9

stvstnfrd added 5 commits February 19, 2015 19:56

Fix Pylint: W0105(pointless-string-statement)

3ffa967

Fix Pylint: E0101(return-in-init)

d21fdaa

Lower Pylint threshold by 200 violations

9900814

Remove useless test

e7b5ada

Per @sarina: > `build_problem` is called in `_make_problem`, which other tests call > in this class. A failure here will arise in other tests. This test is > unnecessary.

Add TODO comment about future test improvements

237f2c3

stvstnfrd force-pushed the pylint/low branch from 6a0c0b1 to 237f2c3 Compare February 20, 2015 03:57

sarina mentioned this pull request Feb 20, 2015

Fix Pylint violations (easy) #7049

Closed

sarina added a commit that referenced this pull request Feb 20, 2015

Merge pull request #7013 from stvstnfrd/pylint/low

6900bd2

Fix Pylint violations (easy)

sarina merged commit 6900bd2 into openedx:master Feb 20, 2015

stvstnfrd deleted the pylint/low branch February 20, 2015 21:57

Uh oh!

Conversation

stvstnfrd commented Feb 18, 2015

Uh oh!

openedx-webhooks commented Feb 18, 2015

Uh oh!

singingwolfboy Feb 18, 2015

Choose a reason for hiding this comment

Uh oh!

stvstnfrd Feb 18, 2015

Choose a reason for hiding this comment

Uh oh!

sarina Feb 18, 2015

Choose a reason for hiding this comment

Uh oh!

singingwolfboy commented Feb 18, 2015

Uh oh!

stvstnfrd commented Feb 18, 2015

Uh oh!

sarina commented Feb 18, 2015

Uh oh!

sarina commented Feb 18, 2015

Uh oh!

stvstnfrd commented Feb 18, 2015

Uh oh!

stvstnfrd commented Feb 18, 2015

Uh oh!

sarina Feb 18, 2015

Choose a reason for hiding this comment

Uh oh!

sarina Feb 18, 2015

Choose a reason for hiding this comment

Uh oh!

sarina commented Feb 19, 2015

Uh oh!

stvstnfrd commented Feb 19, 2015

Uh oh!

sarina commented Feb 20, 2015

Uh oh!

sarina commented Feb 20, 2015

Uh oh!

sarina commented Feb 20, 2015

Uh oh!

stvstnfrd commented Feb 20, 2015

Uh oh!

sarina commented Feb 20, 2015

Uh oh!

sarina commented Feb 20, 2015

Uh oh!

sarina commented Feb 20, 2015

Uh oh!

stvstnfrd commented Feb 20, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants