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

crypto-square exercise tests contradict README.md #458

Closed
Phippsaurus opened this issue Mar 11, 2018 · 7 comments · Fixed by #488
Closed

crypto-square exercise tests contradict README.md #458

Phippsaurus opened this issue Mar 11, 2018 · 7 comments · Fixed by #488

Comments

@Phippsaurus
Copy link

In the README.md for the exercise crypto-square it says:

Output the encoded text in chunks. Phrases that fill perfect rectangles
(r X c) should be output c chunks of r length, separated by spaces.
Phrases that do not fill perfect rectangles will have n empty spaces.
Those spaces should be distributed evenly, added to the end of the last
n chunks.

The canonical data gives an example for this:

    {
      "description": "54 character plaintext results in 7 chunks, the last two with trailing spaces",
      "property": "ciphertext",
      "input": {
        "plaintext": "If man was meant to stay on the ground, god would have given us roots."
      },
      "expected": "imtgdvs fearwer mayoogo anouuio ntnnlvt wttddes aohghn  sseoau "
}

Contrarily the rust implementation of that test looks like this:

fn test_example() {
    test(
        "If man was meant to stay on the ground, god would have given us roots.",
        "imtgdvs fearwer mayoogo anouuio ntnnlvt wttddes aohghn sseoau",
    )
}

I was about to create a pull request for that issue, but there are further discrepancies which are not quite clear. The rust tests strip numbers from the input, where the canonical data leaves them in. Furthermore the rust version strips non-ASCII characters, where the canonical data has no example for that case.

If somebody could clarify what the exercise is actually supposed to look like, I will adjust it accordingly.

@dantho
Copy link

dantho commented Mar 12, 2018

lines 50-52 in the README use the word "spaces" but really mean missing characters or short lines. The requirement is to distribute the jaggedness so the encoded rows at the bottom are at most 1 char short of the first row(s).

I had trouble with this, so I made my own test_jagged() test.

test("aaaa bbbb cccc d", "abcd abc abc abc");
test("aaaa bbbb cccc dd", "abcd abcd abc abc");
test("aaaa bbbb cccc ddd", "abcd abcd abcd abc");

If I can figure out how, I will do a pull request with this clarification and added tests.

@Phippsaurus
Copy link
Author

I appreciate you looking into this so quickly, but I think it would be less effortful to adapt the Rust tests and example implementation, rather than changing the README.md which would affect all other language tracks as well.

For example, the F# version also tests for added spaces:

[<Fact(Skip = "Remove to run test")>]
let ``8 character plaintext results in 3 chunks, the last one with a trailing space`` () =
    ciphertext "Chill out." |> should equal "clu hlt io "

[<Fact(Skip = "Remove to run test")>]
let ``54 character plaintext results in 7 chunks, the last two with trailing spaces`` () =
    ciphertext "If man was meant to stay on the ground, god would have given us roots." |> should equal "imtgdvs fearwer mayoogo anouuio ntnnlvt wttddes aohghn  sseoau "

I can't color highlight here, but notice how the shorter blocks are padded with spaces in addition to those required to separate the blocks.
That is in accordance with the language-independent canonical data and the README.md as well as description.md.

@emengd
Copy link

emengd commented Mar 13, 2018

Hi, found this as I just did the exercise and saw the same problem.

It seems to me that the original Readme was very clear about what it wanted, and it's the tests that are wrong. Your new readme doesn't make any sense to me: if you add the last characters to the last chunks, they will be longer, not shorter than the first. What your tests instead enforce is that the last characters be added to the first chunks. Also, the phrasing as "adding the missing characters" rather than "adding spaces" suggest a different definition for r and c, though honestly attempting to completely think this through gives me a headache.* Anyway what your tests enforce is very much not what the original readme wanted, which is that spaces should be appended to the string so that it fills the rectangle, as you can see in the example.

(*In fact, but this is not specific to the Rust track I think, the Readme doesn't specify really explicitly how to choose r and c. What you have to pick is the smallest (r, c) that fits the message, which is reasonably easy to guess from the example. It is less easy to guess with your phrasing, because we're not sure anymore whether the r x c rectangle is the one that encloses the padded message, or the one that encloses the truncated message.)

@petertseng
Copy link
Member

petertseng commented Mar 16, 2018

The rust tests strip numbers from the input, where the canonical data leaves them in.

OK, I think the Rust test is in the wrong here. https://github.com/exercism/problem-specifications/blob/master/exercises/crypto-square/description.md tells me that "the spaces and punctuation are removed from the English text", and I think we agree that a number is not a space or a punctuation. Numbers should be left in the text. This means that any tests that expects numbers to be removed should be changed.

Furthermore the rust version strips non-ASCII characters, where the canonical data has no example for that case.

The Exercism-wide policy in exercism/problem-specifications#428 would have us not use non-ASCII characters at all. In instances such as scrabble-score (#421 and #125) and reverse-string (#428) we chose to contravene the policy where it is essential to the problem.

I do not think the behavior is essential to the problem, since the act of performing the transposition is the same no matter whether you are transposing individual bytes, grapheme clusters, or even any other data type that doesn't necessarily need to map to readable text.

I thus recommend those cases be removed.

Phrases that do not fill perfect rectangles will have n empty spaces.
Contrarily the rust implementation of that test looks like this:

I agree that the Rust implementation does not agree with the specification when dealing with trailing spaces.
I agree that the Rust implementation should be change such that it agrees with the specification.

The solution I suggested to spacing in exercism/problem-specifications#190 was to remove all spacing so that the output would be "imtgdvsfearwermayoogoanouuiontnnlvtwttddesaohghnsseoau" but that horse has left the stable... for now. If there were a case for it, it could be changed.

@dantho
Copy link

dantho commented Mar 21, 2018

I've come to appreciate @Phippsaurus 's OP. I hope my eagerness to contribute hasn't muddied the waters too much.
@petertseng , I agree with your assessment and recommendations. (But I think that "horse" at the bottom should be left in peace. By that I mean cramming the encoded output into one long string without spaces looks very confusing to me.)

At present, I have PR open against the specification, to clarify the description.md, and a misguided PR open against Rust, which should probably just be rejected. How do I revoke a PR?

@petertseng
Copy link
Member

How do I revoke a PR?

If you visit https://github.com/exercism/rust/pull/459 (#459) you should see a Close pull request button. It is at the bottom of the page in the browser.

@dantho
Copy link

dantho commented Mar 21, 2018

Closed. Thanks.

coriolinus added a commit to coriolinus/exercism_rust that referenced this issue Apr 4, 2018
The initial implementation of this exercise diverged from the
README: it stripped out the extra padding spaces specified, and
it eliminated all non-alphabetical characters instead of just
getting rid of punctuation.

This commit rectifies both of those diversions. This closes exercism#458.
However, in doing so it uses a method only introduced in Rust 1.24;
only those people who are relatively recent in their Rust compilers
will be able to compile the example.

As the example is not primarily for students, this should be acceptable.
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 a pull request may close this issue.

4 participants