-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Fix Pylint violations (easy) #7013
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
75aaf0d
416f709
2e1400f
ee2404c
e17504d
66f28fd
49fb966
a43cd3f
c6ef359
69695a8
c0a7bf9
3ffa967
d21fdaa
9900814
e7b5ada
237f2c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -706,7 +706,7 @@ def make_course_key(self, org, course, run): | |
| """ | ||
| return CourseLocator(org, course, run, deprecated=True) | ||
|
|
||
| def get_courses(self, depth=0, **kwargs): | ||
| def get_courses(self, **kwargs): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we certain that nothing relies on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My extensive
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found this hard to believe at first, but looking at the function signature for other definitions of |
||
| """ | ||
| Returns a list of course descriptors. If there were errors on loading, | ||
| some of these may be ErrorDescriptors instead. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would somebody have added this line? It's truly doing nothing, right? Is the test even doing anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anything test anything!?
But yeah, it reminds me of something I saw recently (paraphrased):
It's like, just remove the first 2 checks and if it blows up, GOOD! That's what we want to know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really doesn't test much though. It would be better to like, instantiate the XML as further problems do (
rendered_html = etree.XML(problem.get_html())) and then actually assert something about the rendered_html itself.I'm fine if you don't want to do that here, but if you don't want to do that, please add a TODO eg
TODO: This test should inspect the rendered html and assert one or more things about it