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

9441 conch public key #1010

Merged
merged 7 commits into from
Jul 8, 2018
Merged

9441 conch public key #1010

merged 7 commits into from
Jul 8, 2018

Conversation

markrwilliams
Copy link
Member

Contributor Checklist:

This makes the implementation match its docstring, which says that
Key.public() may return the same object if it is already a public key.
@codecov
Copy link

codecov bot commented May 10, 2018

Codecov Report

Merging #1010 into trunk will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            trunk    #1010      +/-   ##
==========================================
+ Coverage   91.88%   91.89%   +<.01%     
==========================================
  Files         842      842              
  Lines      150197   150198       +1     
  Branches    13138    13137       -1     
==========================================
+ Hits       138008   138018      +10     
+ Misses      10096    10091       -5     
+ Partials     2093     2089       -4

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this.

Looks good to merge.

I have reverted the fix and run only the tests and I can see a failure.

In terms of testing, I was expecting to see another test with dedicated docstring like

def test_publicAlreadyPublic():
    """
    For public keys, it returns the same instance.
    """
    publicRSAKey = keys.Key.fromString(keydata.publicRSA_openssh)
    self.assertIs(publicRSAKey, publicRSAKey.public())

@wiml
Copy link
Contributor

wiml commented Jul 6, 2018

@adiroiban : I think that having public_key() return self is only an implementation detail; at least, that's what the documentation says. I don't think the unit tests should be asserting something that the documentation explicitly says you shouldn't rely on.

@adiroiban adiroiban merged commit b604217 into trunk Jul 8, 2018
@adiroiban
Copy link
Member

I don't think the unit tests should be asserting something that the documentation explicitly says you shouldn't rely on.

True.. Thanks. Merged

@adiroiban adiroiban deleted the 9441-conch-public-key branch August 1, 2021 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants