-
-
Notifications
You must be signed in to change notification settings - Fork 556
Add transpose test cases #320
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
Conversation
I like what I see for integers. The exercise description and source specifiy text as well, how do we want to handle that? There is a PR on xRust that assume text inputs, and the additional implication here is that the rows/columns need not all be equal sizes. Some additional cases to deal with those would probably be appropriate. |
Good idea, I'll add some string cases too that take into account varying row/column lengths. |
@petertseng Having read the exercise, it does indeed explicitly mention text. If we would also like to support transposing non-string arrays, I think it makes sense to have a multiline string passed as an array of strings (each line being an array entry). What do you think? |
Is it likely that a solution that solves for arrays of numbers will not also solve arrays of strings? |
@Insti Yes, but it is not clear from the exercise text that the string is passed as an array of strings instead of a string with newline characters. |
The description:
Would seem to suggest that the input (and output) is text with newlines which would make the arrays in the .json file inappropriate? Edit to add:
Given the input and output are expected to be text, I strongly disagree with this. |
@petertseng @Insti Maybe we should then only focus on text and not on integers? |
Yes, probably. But it looks like fsharp is the only track that implements this problem? |
@Insti Well, I was coming around to that :) The only thing that would be modified is that multiple lines are passed as an array of strings. |
Opinion: |
@Insti Agreed. I'll close this PR. |
A Word square would make an interesting test case.
|
And Word rectangles (from the same page)
|
@Insti On second thought, maybe I should update the PR with some new, string-based examples? |
@ErikSchierboom I think that's a good idea. |
e6b4547
to
2548a67
Compare
@Insti @petertseng I've updated the test cases to work with strings. Better? |
"JSON doesn't allow for multi-line strings, so all multi-line input is ", | ||
"presented here as arrays of strings. It's up to the test generator to join the ", | ||
"lines together with line breaks." | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{"multi_line_string":"Multi\nLine\nString\n"}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it should be: JSON doesn't allow for elegant formatting of multi-line strings, so all multi-line input and output is
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a good wording. I copied the text from the existing JSON data from food-chain by the way.
This new version looks great 👍 It would be good with some simple initial test cases.
and
and their inverses. When doing an exercise, the less output I need to look through to work out what might be wrong the better. |
Good idea. Will add them. |
2548a67
to
31ca137
Compare
@Insti done! |
"ni", | ||
"en", | ||
".e", | ||
"." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the original text, the period belongs to the second line, so there should be a space before it in the transposed text
(That would be consistent with what the source https://www.reddit.com/r/dailyprogrammer/comments/4msu2x/challenge_270_easy_transpose_the_input_text says, and the PR exercism/rust#151 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fixed in the updated PR.
31ca137
to
466a2f7
Compare
There doesn't seem to be a test case where the first line is longer than the second line. |
f8f3172
to
e00effe
Compare
@Insti @petertseng I've updated the incorrect test cases, and added a test case where the first line is longer than the second line. I've also looked at the xRust PR and it appears their test case also had some problems, where input spaces where stripped in the output. |
I think everything is in order (I haven't verified with code though). It might be worth adding to the README a few things, because otherwise the problem seems unspecified.
|
e00effe
to
31716f4
Compare
@petertseng The README has been updated as suggested. Is it clear enough? |
|
||
And transposing: | ||
|
||
Therefore, transposing this matrix: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this line, I think
I like what I see here (with a few small fixes) 👍 from me once the fixes are applied |
31716f4
to
d906451
Compare
@petertseng I've fixed the remaining issues and I'll merge it. Thanks for the help! |
" o n", | ||
" v d", | ||
" e .", | ||
" , " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I missed a small thing - no spaces after the comma, right?
(I also haven't checked with code, so I may have missed others, but I least see this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll send the PR. This was the only thing.
No description provided.