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

simple-cipher: Add canonical data #966

Closed
wants to merge 4 commits into from
Closed

simple-cipher: Add canonical data #966

wants to merge 4 commits into from

Conversation

aos
Copy link
Contributor

@aos aos commented Oct 21, 2017

Closes #586

Hello. I've been working on this one. There are a few places where I wasn't sure what to do, specifically L27, and others where you have to do multiple encode into decodes. Or when adding a new cipher key. Let me know what you think!

@ilya-khadykin
Copy link
Contributor

@aos build failed due to:

exercises/simple-cipher/canonical-data.json invalid
[ { keyword: 'oneOf',
 dataPath: '.cases[0]',
 schemaPath: '#/oneOf',
 params: {},
 message: 'should match exactly one schema in oneOf' } ]

@Insti Insti changed the title Added canonical data for simple cipher simple-cipher: Add canonical data Oct 21, 2017
@Insti
Copy link
Contributor

Insti commented Oct 21, 2017

@Insti
Copy link
Contributor

Insti commented Oct 31, 2017

Hi @aos have you had a chance to look into why the json file is not valid?

@aos
Copy link
Contributor Author

aos commented Oct 31, 2017

Hey @Insti. I looked at it briefly but couldn't understand. Sorry I have been very busy with work. I will look at it more tonight when I get a chance!

@Insti
Copy link
Contributor

Insti commented Oct 31, 2017

Try pasting the json into a schema checker for some clues:

https://jsonschemalint.com/#/version/draft-04/markup/json?gist=ba6a1e74397ade6bdbff65a9df0d9d3f

@aos
Copy link
Contributor Author

aos commented Oct 31, 2017

Oh... that's handy. I'll look through it in a bit. Thank you!

@ErikSchierboom
Copy link
Member

@aos Did you find any time to look at the problem? Do you perhaps need any help?

@aos
Copy link
Contributor Author

aos commented Nov 14, 2017

@ErikSchierboom @Insti Hey finally got around to it. The latest commit fixes the schema validation issue. Let me know what still needs work -- I'll eventually squash these into a single commit

Copy link
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

I see all of input, input_plaintext, input_string, input_encode, input_all_caps_string, input_all_numbers_string, and possibly others in the cases. I also see output_key, matches_regex, output_string, output_decode, and output_error; there are again possibly others that I've missed.

I haven't gone through to check that all items with the same property have the same inputs and outputs; if they do, that's technically not wrong. Still, it's both better aesthetics and easier to automatically generate tests with if every test case has the same schema:

{
   "property": "Some string",
   "description": "Text describing what this case is checking",
   "input": "any literal",
   "expected": {
      "value": "success value (any type)"
// or "error": "error type"
// or "matches_regex": "regex"
   }
}

In the above example, you can assign whatever attributes you like to the expected object, as is appropriate for the test case; as long as every test of the same property has the same object attributes, things work out well. Likewise, you can set the input to whatever you like, as long as the types are consistent within a given property. The key is consistency.

@coriolinus
Copy link
Member

you can assign whatever attributes you like to the expected object

cf. the Readme:

There are also some convention about expected that you must follow:

  • if the input is valid but there is no result for the input, the value at "expected" should be null.
  • if an error is expected (because the input is invalid, or any other reason), the value at "expected" should be an object containing exactly one property, "error", whose value is a string.
    • The string should explain why the error would occur.
    • A particular track's implementation of the exercise need not necessarily check that the error includes that exact string as the cause, depending on what is idiomatic in the language (it may not be idiomatic to check strings for error messages).

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

I've added some remarks. Don't be daunted, most of it is relatively simple and involves being consistent with other exercises. It might help to look at some other exercises like https://github.com/exercism/problem-specifications/blob/master/exercises/atbash-cipher/canonical-data.json.

Thanks a lot for working on this!

"cases": [
{
"description": "Random key cipher",
"property": "random",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, but I don't think this line actually does anything. The property key is normally used for specifying which method/property to test. As this is just a container for individual cases, I think it can be removed.

},
{
"description": "Incorrect key cipher",
"property": "incorrect",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, but I don't think this line actually does anything. The property key is normally used for specifying which method/property to test. As this is just a container for individual cases, I think it can be removed.

},
{
"description": "Substitution cipher",
"property": "substitution",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, but I don't think this line actually does anything. The property key is normally used for specifying which method/property to test. As this is just a container for individual cases, I think it can be removed.

"description": "It has a key made of letters",
"property": "key",
"input": "any",
"matches_regex": "^[a-z]+$"
Copy link
Member

Choose a reason for hiding this comment

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

We are in the process of enforcing lowerCamelCase keys (see #987), so I would suggest to rename the keys (not only this one) to lowerCamelCase.

"property": "random",
"cases": [
{
"description": "It has a key made of letters",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe shorten to "Key has only lower-case letters"?

},
{
"description": "Substitution cipher",
"property": "substitution",
Copy link
Member

Choose a reason for hiding this comment

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

Should this be encode?

Copy link
Member

Choose a reason for hiding this comment

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

Possibly it should be substitution_encode or substitutionEncode, if there are other encoding methods with different types of inputs and outputs.

{
"description": "It throws an error with a numeric key",
"property": "Cipher Key",
"input_all_number_string": "'12345'",
Copy link
Member

Choose a reason for hiding this comment

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

Rename to input

},
{
"description": "It throws an error with a numeric key",
"property": "Cipher Key",
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 the property should be key

"cases": [
{
"description": "It throws an error with an all caps key",
"property": "Cipher Key",
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 the property should be key

"property": "incorrect",
"cases": [
{
"description": "It throws an error with an all caps key",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this test should be about testing a string with a single uppercase character, as that is the minimal counter-example.

@Insti
Copy link
Contributor

Insti commented Nov 23, 2017

Are you still working on this @aos ? Do you need any help?

@aos
Copy link
Contributor Author

aos commented Nov 24, 2017

Hi everyone. My apologies -- I've been very busy at work and have not had the time to work on this. If someone else would like to takeover, I'm more than happy to pass it on. Otherwise, I'll try to make progress on it as I collect some free time.

@coriolinus
Copy link
Member

@aos That's no problem; take the time you need to. There's no deadline!

@ErikSchierboom
Copy link
Member

@aos Just a small bump. Are you still interested in working on this? I totally understand if work has prevented you to work on this, but this is just a small, friendly reminder :)

@ErikSchierboom
Copy link
Member

What should we do with this PR? It has been 29 days since my reminder, and unfortunately there has been no reply. Is someone else perhaps willing to continue working on this PR?

@rpottsoh
Copy link
Member

rpottsoh commented Mar 3, 2018

Would this PR simply be closed and a new one opened? Would it matter if one decided to start over from scratch? I am not putting my hat in the ring at this point, but I have spent about 90 minutes playing around with the JSON and looking at the recommended changes and finding it difficult to make sense of this JSON and have pass linting after making the prescribed changes.

@ErikSchierboom
Copy link
Member

That may be the best option indeed.

@ErikSchierboom
Copy link
Member

I'm gonna go ahead and close this PR. If someone wants to continue working on it, that would be great! But please open another PR.

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.

6 participants