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

Rbarlow ssl verification coverage #537

Merged
merged 2 commits into from Aug 11, 2014

Conversation

bowlofeggs
Copy link

No description provided.

@bmbouter bmbouter self-assigned this Aug 11, 2014
"""
Tests for the RepoHandler object.
"""
@mock.patch('pulp_rpm.handlers.bind.repolib.bind')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a blank line above this to separate it logically from the docstring.

Copy link
Author

Choose a reason for hiding this comment

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

I actually prefer this style, but that's partly because vim makes it look really nice. The real reason is that I really hate wasting vertical space because even with the resolution I have I like having higher information density on my screen when I read. Bet you couldn't guess that in C I really hate when people put curly braces on their own line.

I don't believe that PEP-8 makes a statement one way or the other, but if it does I'd be happy to follow it. Otherwise I'd personally prefer to maintain more information per line of code ☺

@bmbouter
Copy link
Member

The tests introduced in test_repolib.py contain a lot of duplicate code. Consider de-duplicating it using helper functions. This would let the unique assertions of each test stand out more.

@bmbouter
Copy link
Member

code coverage is good, PEP8 compliance is good, and all tests pass. Merge at will.

@bowlofeggs bowlofeggs merged commit 686e63e into master Aug 11, 2014
@bowlofeggs bowlofeggs deleted the rbarlow-ssl_verification_coverage branch August 11, 2014 20:38
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