Skip to content

matrix: Remove string tests #543

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

Merged
merged 1 commit into from
May 16, 2017
Merged

matrix: Remove string tests #543

merged 1 commit into from
May 16, 2017

Conversation

rbasso
Copy link
Contributor

@rbasso rbasso commented May 12, 2017

It was argued in #541 that the the exercise would be more interesting without the fromString function, which is not an essential part of of it.

  • Rewrite test suite to use [[Int]] instead of String.
  • Remove test that wouldn't make sense without fromString.
  • Remove all other references to fromString.
  • Fix HINTS.md to remove reference to invalid characters.

Edit:

The scope of this PR was restricted to just removing the last three tests, based on the discussion below. If in the future we decided to completely remove fromString from the test suite, description.md will demand some rewriting.

I think that it is preferable to completely rewrite or remove this exercise. The way it is now is lacks focus and also exposes users to a really unidiomatic interface.


Closes #541.

@petertseng
Copy link
Member

I drew a different conclusion from #541 - I thought that the parsing (in particular, parsing a matrix of ints) was okay, it was simply the tests of parsing a matrix of strings (the two that say "may be tricky") that should have been removed. The matrix of chars probably could go too.

The https://github.com/exercism/x-common/blob/master/exercises/matrix/description.md does mention "given a string with embedded newlines", which does imply that the string parsing was an intended part... just probably not matrices containing non-numbers.

It is true that I wouldn't mind removing the parsing completely (there is already a lot in this exercise) and leaving it for another exercise - please indicate if that is an intended route.

@rbasso
Copy link
Contributor Author

rbasso commented May 14, 2017

Oh! When I was reading the exercise I thought that having fromString and representing all matrices as strings in the test suite was so ugly in Haskell that I almost automatically assumed that the complaint was about it.

Right now I'm a little busy, but I can rewrite the PR in a in a day or two.

If anyone want to fix it before that, fell free to open another PR and close this one.

@rbasso
Copy link
Contributor Author

rbasso commented May 16, 2017

PR Updated to just remove the three final tests.

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.

you commented saying remove the final three, but removed only the final two. however, the commit contents are consistent with the commit message (removing tricky string). intentional?

@rbasso
Copy link
Contributor Author

rbasso commented May 16, 2017

you commented saying remove the final three, but removed only the final two.

Nice catch, @petertseng. Thanks for checking that!

I guess I forgot to push after deciding to remove the third one.

however, the commit contents are consistent with the commit message (removing tricky string). intentional?

I considered that the third one was also in the trick, so I'm keeping the description. Ok?

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.

indeed (now only matrices of numbers)

@rbasso rbasso merged commit f006e4c into exercism:master May 16, 2017
@rbasso rbasso deleted the matrix-remove-fromString branch May 16, 2017 19:25
@petertseng
Copy link
Member

We forgot to increment the test version (the fourth component)

@rbasso rbasso mentioned this pull request May 16, 2017
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.

2 participants