Add transpose#466
Conversation
|
Super, Thanks ❤️ |
| "isogram" | ||
| "isogram", | ||
| "transpose" | ||
| ], |
There was a problem hiding this comment.
Added to the end, great, that is the right place to put new problems. 👍
| <%= test_case.skipped %> | ||
| expect = <%= test_case.expect %> | ||
| actual = <%= test_case.work_load %> | ||
| assert_equal expect, actual |
There was a problem hiding this comment.
I really like the 3 line format you're using here.
I'd call it expected but expect is ok too.
| end | ||
|
|
||
| def work_load | ||
| "Transpose.transpose(#{input_text.dump})" |
There was a problem hiding this comment.
Nice use of dump to handle escaping.
|
This looks good ❤️ 🤔 I wonder if there's some way of making the tests better show the transposed results. Edit: I guess the test failure messages show it a bit better so maybe we don't need to do anything different in the test_cases file. |
gchan
force-pushed
the
transpose
branch
2 times, most recently
from
657d1d1 to
ff05173
Compare
|
Thanks @Insti, I've renamed the variable Could potentially use HEREDOC or string concatenation with |
gchan
force-pushed
the
transpose
branch
3 times, most recently
from
c47a989 to
e9ec73b
Compare
If you want to try it and see how it looks that would be good. You could add other temporary files to the PR called Another thing we could do is add a comment to the test_cases.rb file asking people to submit an issue if they have an idea for a better way to display it. |
|
Added files for a heredoc version for test generation. 😄 |
|
Ok, Thanks for doing that ❤️ The initial version is my preferred one. |
kotp
left a comment
kotp
left a comment
There was a problem hiding this comment.
Heredoc indentation is a good thing, and not hard to implement. I think we will need to, as I think we are still using Ruby 2.2.
| input = <<-INPUT | ||
| ABC | ||
| 123 | ||
| INPUT |
There was a problem hiding this comment.
With a tilde ~ in front of the Heredoc delimiter, you can present these heredocs with the proper indentation, and they will end up looking good here and clean in the generated files. Including indentation in side the text. This does require Ruby 2.3 though, but it is not a hard thing to write. Facets gem has this functionality for a fairly long time (I wrote one in 2008, unsure if Facets got my version).
There was a problem hiding this comment.
The tilde for heredocs in Ruby 2.3 is nice feature. Thanks for the tip!
|
@Insti You still prefer the first version, or did the heredocs change do it for you? |
|
The indented heredocs are much nicer. |
| [ | ||
| "<<-EXPECTED.gsub(/^ {6}/, '')", | ||
| indent_lines(expected), | ||
| indent_line('EXPECTED') |
There was a problem hiding this comment.
I'd extract an indented_heredoc method.
There was a problem hiding this comment.
Replace indent_line('EXPECTED') with `indented_heredoc('EXPECTED')?
There was a problem hiding this comment.
No. replace
"<<-EXPECTED.gsub(/^ {6}/, '')",
indent_lines(expected),
indent_line('EXPECTED')
with
indented_heredoc(expected)
There was a problem hiding this comment.
Create a helper method to indent those things, all of them are by 6, that is the argument to the method.
<<-EXPECTED.trim_to(6)
There was a problem hiding this comment.
Please see latest changes. Not sure if there is a trim_to method for Ruby String?
There was a problem hiding this comment.
There could be. :) You can make one. That is what I was saying. The name would probably be left_trim though to be clear.
There was a problem hiding this comment.
For this to work I will have to monkey patch the String class with a left_trim method in the generated tests?
There was a problem hiding this comment.
Refinements may be an approach as well.
There was a problem hiding this comment.
Consider that this is not critical. So this should be something to think about, and play with. The duplication here should not stop this PR.
|
Thanks for all your work on this @gchan ❤️ |
|
No problem. Thanks for the feedback. I learnt a few things along the way |
3 participants
Test generator for the transpose exercise and example solution.