Add tests and type hints to hill_cipher.py #1862
Conversation
There was a problem hiding this comment.
Awesome! Thanks for doing this. I changed the title so that reviewers know what this pull request does without having to find, open up, and read an issue. The commit Submodule Python added at 8f2c99 must be removed before this PR can be landed. Tons of comments but we really appreciate this PR and want to see it landed.
| """ | ||
| self.encrypt_key = self.modulus(encrypt_key) # mod36 calc's on the encrypt key | ||
| self.check_determinant() # validate the determinant of the encryption key | ||
| self.decrypt_key = None | ||
| self.break_key = encrypt_key.shape[0] | ||
|
|
||
| def replace_letters(self, letter): |
There was a problem hiding this comment.
Need type hints. This function does not really replace anything. Can we rename to something more self documenting? get_letter_by_letter or lookup_letter_by_letter. Alternatively could we have a single function instead of two functions? get_letter or lookup_letter could start with isinstance(key, int) and return the right result if key was a str or int.
| batch_vec = numpy.matrix([batch_vec]).T | ||
| batch_encrypted = self.modulus(self.encrypt_key.dot(batch_vec)).T.tolist()[ | ||
| 0 | ||
| ] | ||
| encrypted_batch = "".join(list(map(self.replaceNumbers, batch_encrypted))) | ||
| encrypted_batch = "".join( | ||
| list(map(HillCipher(self.encrypt_key).replace_numbers, batch_encrypted)) |
There was a problem hiding this comment.
Please make this a list comprehension as discussed in CONTRIBUTING.md.
| """ | ||
| return self.key_string.index(letter) | ||
|
|
||
| def replace_numbers(self, num): |
There was a problem hiding this comment.
Type hints? Combine with previous function? Or replace with __index__().
| >>> | ||
| """ | ||
| text: list = list(text.upper()) | ||
| text: list = [char for char in text if char in self.key_string] |
There was a problem hiding this comment.
It is too confusing to use the same variable as both a str and as a list. Can we replace lines 113 and 114 with:
chars = [char for char in text.upper() if char in self.key_string]
and then change the variable name in the lines below? We do not need to declare the type because CPython is smart enough to know that a list comprehension returns a list.
| @@ -94,26 +120,37 @@ def process_text(self, text): | |||
| return "".join(text) | |||
|
|
|||
| def encrypt(self, text): | |||
There was a problem hiding this comment.
| def encrypt(self, text): | |
| def encrypt(self, text: str) -> str: |
| batch_vec = numpy.matrix([batch_vec]).T | ||
| batch_decrypted = self.modulus(self.decrypt_key.dot(batch_vec)).T.tolist()[ | ||
| 0 | ||
| ] | ||
| decrypted_batch = "".join(list(map(self.replaceNumbers, batch_decrypted))) | ||
| decrypted_batch = "".join( | ||
| list(map(HillCipher(self.encrypt_key).replace_numbers, batch_decrypted)) |
| decrypted += decrypted_batch | ||
|
|
||
| return decrypted | ||
|
|
||
|
|
||
| def main(): | ||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Doctests do not work in if __name__ == "__main__": so please bring back main().
| N = int(input("Enter the order of the encryption key: ")) | ||
| hill_matrix = [] | ||
| hill_matrix: list = [] |
There was a problem hiding this comment.
The type hint is not really required here because CPython is smart enough to know that [] is a list. A useful type hint would be hill_matrix: List[int] = [] assuming that from typing import List was added above.
| @@ -42,6 +42,10 @@ | |||
|
|
|||
|
|
|||
| def gcd(a, b): | |||
There was a problem hiding this comment.
Please add type hints to function parameters and return values. This is where type hints are most valuable/helpful because in other locations CPython is often smart enough to determine the type from the context.
| def gcd(a, b): | |
| def gcd(a: int, b: int) -> int |
| @@ -84,8 +105,13 @@ def check_determinant(self): | |||
| ) | |||
|
|
|||
| def process_text(self, text): | |||
There was a problem hiding this comment.
| def process_text(self, text): | |
| def process_text(self, text: str) -> str: |
|
Thanks @cclauss apologies for the delay. I will work to get these requested changes made 👍 |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Describe your change:
Adding tests for issue Fixes: #1788
Checklist:
Fixes: #{$ISSUE_NO}.