Skip to content

crypto-square: apply lowerCamelCase policy #1168

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

Closed
wants to merge 1 commit into from

Conversation

rpottsoh
Copy link
Member

@rpottsoh rpottsoh commented Feb 4, 2018

per #987

@rpottsoh rpottsoh self-assigned this Feb 4, 2018
"cases": [
{
"description": "empty plaintext results in an empty ciphertext",
"property": "ciphertext",
"description": "empty plainText results in an empty cipherText",
Copy link
Member

Choose a reason for hiding this comment

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

I assume you deliberately chose to use the property names, but I think it would maybe be better to use plain text and cipher text here.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed descriptions to not use property names in text.

@rpottsoh rpottsoh force-pushed the updateCrypto-Square branch 2 times, most recently from 3e1a63f to 3b88936 Compare February 5, 2018 15:30
Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

so, do you mind if I ask for these to remain ciphertext and plaintext?

https://en.wikipedia.org/wiki/Plain_text is significantly different from https://en.wikipedia.org/wiki/Plaintext , and I believe plaintext is more applicable to the situation at hand (cryptography).

https://en.wikipedia.org/wiki/Cipher_text redirects to https://en.wikipedia.org/wiki/Ciphertext

@rpottsoh
Copy link
Member Author

rpottsoh commented Feb 5, 2018

Not at all @petertseng. Thanks for the feedback.

@rpottsoh rpottsoh force-pushed the updateCrypto-Square branch from 3b88936 to 2982fbd Compare February 5, 2018 17:33
@rpottsoh
Copy link
Member Author

rpottsoh commented Feb 5, 2018

@petertseng you were only referring to their use in the descriptions right?

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

I'll show an example for one test case; consider me to say the same for all of them.

"cases": [
{
"description": "empty plaintext results in an empty ciphertext",
"property": "ciphertext",
"property": "cipherText",
Copy link
Member

Choose a reason for hiding this comment

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

I think ciphertext is a better property than cipherText, so I would revert this change.

"input": {
"plaintext": ""
"plainText": ""
Copy link
Member

Choose a reason for hiding this comment

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

I think plaintext is a better input key than plainText, so I would revert this change.

@@ -1,60 +1,60 @@
{
"exercise": "crypto-square",
"version": "3.2.0",
"version": "3.2.1",
"cases": [
{
"description": "empty plaintext results in an empty ciphertext",
Copy link
Member

Choose a reason for hiding this comment

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

I think this is good. I think "plaintext" and "ciphertext" are better in descriptions than "plain text" and "cipher text"

@rpottsoh
Copy link
Member Author

rpottsoh commented Feb 5, 2018

Closing this PR. Citing https://en.wikipedia.org/wiki/Plaintext. For this exercise "plaintext" and "ciphertext" are treated as single words.

@rpottsoh rpottsoh closed this Feb 5, 2018
@rpottsoh rpottsoh deleted the updateCrypto-Square branch February 6, 2018 15:15
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.

3 participants