Skip to content
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

Fix pyflakes and pep8 errors #229

Merged
merged 1 commit into from Dec 6, 2015
Merged

Fix pyflakes and pep8 errors #229

merged 1 commit into from Dec 6, 2015

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Nov 25, 2015

Use extra asserts to use previously unused variables in tests,
such as cass and response.

Fix only pyflakes errors in docs/conf.py

@jayvdb
Copy link
Contributor Author

jayvdb commented Nov 25, 2015

fwiw, the .travis.yml changes will be mostly unnecessary after #228

@kevin1024
Copy link
Owner

Looks like I caused some conflicts when merging #228. Can you rebase?

Also, I noticed quite a few new asserts. Is this purely for pep8 reasons? Does flake8 require an assert per test or something?

@jayvdb
Copy link
Contributor Author

jayvdb commented Nov 25, 2015

pyflakes requires that all variables are used.
Often the tests had with blah as cass: something which did not access cass. Adding an assert for cass fixes the problem. The alternative is to remove .. as cass:, but I guessed it is more useful to add asserts than remove cass which may be useful for debugging or future asserts.

@kevin1024
Copy link
Owner

Ohh gotcha.

I think I would prefer to do the first option (remove ..as cass) since I prefer to have fewer asserts per test. Asserts without much meaning to the test can dillute the meaning of the important assert. I strive for a single assert per test but often don't achieve it :)

@@ -9,16 +8,21 @@ def test_once_record_mode(tmpdir):
with vcr.use_cassette(testfile, record_mode="once"):
# cassette file doesn't exist, so create.
response = urlopen('http://httpbin.org/').read()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I also change lines like this to urlopen('http://httpbin.org/').read() , and remove the assert response ?

(It is very late here, so I'll pick this up tomorrow and revise it to better suit your tastes.)

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, if we're not doing anything with the variable I suppose we don't need to create it. (BTW thanks a lot for doing this! Having all the code be pep8 gives a nice clean feeling :) 👍

Use extra asserts to use previously unused variables in tests,
such as `cass` and `response`.

Fix only pyflakes errors in docs/conf.py
@kevin1024
Copy link
Owner

Thanks for the cleanup!

kevin1024 added a commit that referenced this pull request Dec 6, 2015
Fix pyflakes and pep8 errors
@kevin1024 kevin1024 merged commit 70c92d0 into kevin1024:master Dec 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants