Skip to content

Fix Pylint violations (medium) - #7063

Merged
sarina merged 11 commits into
openedx:masterfrom
stvstnfrd:pylint/medium
Mar 15, 2015
Merged

Fix Pylint violations (medium)#7063
sarina merged 11 commits into
openedx:masterfrom
stvstnfrd:pylint/medium

Conversation

@stvstnfrd

Copy link
Copy Markdown
Contributor

This is a replacement subset of #7008.

Review commit-by-commit for additional clarity.

@openedx-webhooks

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why this code is Exception.__str__(self, *args, **kwargs) instead of just self? The string formatting should automatically coerce the self argument to a string.

If you don't want to make this change, that's fine with me -- I'm in favor of keeping this pull request small and focused as much as possible. This just caught my eye, and I was confused.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure either, so I think I'll opt to pass for now :)

@singingwolfboy

Copy link
Copy Markdown
Contributor

Looks like there was a problem with this Jenkins build. Jenkins, test this please.

@singingwolfboy

Copy link
Copy Markdown
Contributor

Looks like we're getting a bunch of import errors when running the tests. @stvstnfrd, can you look into this?

@stvstnfrd
stvstnfrd force-pushed the pylint/medium branch 5 times, most recently from cc43f89 to 695914e Compare February 28, 2015 23:54
@stvstnfrd

Copy link
Copy Markdown
Contributor Author

Finally got a green build going; ready for rereview @singingwolfboy

@sarina

sarina commented Mar 2, 2015

Copy link
Copy Markdown
Contributor

@stvstnfrd I'll finsih up the review here.

Can you please rebase to pick up the fixes to Jenkins reporting?

@stvstnfrd

Copy link
Copy Markdown
Contributor Author

@sarina I'm not seeing a Jenkins error (rebased over the weekend).

@sarina

sarina commented Mar 2, 2015

Copy link
Copy Markdown
Contributor

Added a few nits. Good to merge once those are addressed (or argued against) and the PR is rebased.

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed community manager review labels Mar 2, 2015
@sarina

sarina commented Mar 2, 2015

Copy link
Copy Markdown
Contributor

@stvstnfrd the commit that Ben reverted this morning because coverage wasn't properly reported in common/ after the stuff I merged Friday

@sarina

sarina commented Mar 2, 2015

Copy link
Copy Markdown
Contributor

#7187 fyi

@sarina

sarina commented Mar 13, 2015

Copy link
Copy Markdown
Contributor

Needs a rebase, then it's good to merge.

@stvstnfrd

Copy link
Copy Markdown
Contributor Author

@sarina Rebased, clean build, dropped Pylint threshold to 5800 (5739 violations reported).

@sarina

sarina commented Mar 15, 2015

Copy link
Copy Markdown
Contributor

nice!! :D

sarina added a commit that referenced this pull request Mar 15, 2015
@sarina
sarina merged commit fd0e389 into openedx:master Mar 15, 2015
@stvstnfrd

Copy link
Copy Markdown
Contributor Author

Thanks @sarina !

@stvstnfrd
stvstnfrd deleted the pylint/medium branch March 15, 2015 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants