Skip to content

Initial diamond text support in test suite#855

Merged
sshine merged 6 commits intoexercism:masterfrom
chiroptical:diamond_data_text
Oct 21, 2019
Merged

Initial diamond text support in test suite#855
sshine merged 6 commits intoexercism:masterfrom
chiroptical:diamond_data_text

Conversation

@chiroptical
Copy link
Copy Markdown
Contributor

Add Data.Text support for diamond from #841

@sshine Should I just modify the README.md or add a .meta/hints.md similar to https://github.com/exercism/haskell/blob/master/exercises/bob/.meta/hints.md?

Copy link
Copy Markdown
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

You seem to have uncovered a pretty significant problem of #841:

Test suites with property tests don't overload as easily as when we only need to compare for equality.

I can think of three approaches, one of which you have picked:

  • Write a thin ToString class, as you did.
  • Use an existing low-dep package like string-conversions to do the same. (It does pull in bytestring for no purpose beyond letting people solve the exercise with that, too. This exercise only has byte-sized characters in input and output.)
  • Consider going Text-only for this exercise and future exercises in a similar situation. Since diamond is currently unlocked by hello-world, maybe this has implications, and maybe that's okay as long as the stub has the right imports; this isn't set in stone, and it's a principal decision either way.
  • (Edit: In the spirit of my co-maintainer) Or something else entirely.

I am inclined to let you decide, but we can give @petertseng a couple of days to respond.

@sshine
Copy link
Copy Markdown
Contributor

sshine commented Oct 14, 2019

@barrymoo: What would you prefer?

@petertseng
Copy link
Copy Markdown
Member

By the way, just so my position is known:

I agree this is unfortunate. I'm tentatively open to any of the proposed solutions, and don't have any to add at this moment.

I think we're waiting for something to be added to the README, yes?

@sshine
Copy link
Copy Markdown
Contributor

sshine commented Oct 14, 2019

Yes, I'm sorry, I completely forgot that. @barrymoo:

  • Decide which option you prefer to go with
  • Add a .meta/hints.md that resembles e.g. Port raindrops test-suite to support Data.Text #853
  • At the track repo's root, configlet generate . --only diamond
  • Commit and push both of the updated .meta/hints.md and README.md

This will merge the hint with README.md in such a way that passes CI.

@chiroptical
Copy link
Copy Markdown
Contributor Author

@sshine

Decide which option you prefer to go with

I really don't think this is a decision I should make. I am split between string-conversions and Text-only. Many folks just starting out (including me) might prefer to use String because they are comfortable with it. On the other hand, learning and using Text is likely what you will encounter in the "real" world.

@chiroptical
Copy link
Copy Markdown
Contributor Author

The text package actually depends on bytestring anyway (http://hackage.haskell.org/package/text). I think this makes me lean towards the string-conversions package.

@sshine
Copy link
Copy Markdown
Contributor

sshine commented Oct 17, 2019

@barrymoo: I'd go with string-conversions then.

@chiroptical
Copy link
Copy Markdown
Contributor Author

I'll update this PR tonight with string-conversions and metadata.

chiroptical and others added 5 commits October 17, 2019 19:47
Since #874 we've tried to avoid linking to specific versions of Hackage
packages in Markdown files (see justification in #868). This wasn't
enforced while this was worked on.
Copy link
Copy Markdown
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

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

Hi Barry, thanks for your work.

I've pushed two small changes to the feature branch.

I think this is ready to go. As per our recent custom, I'll give other maintainers 48 hours to interject.


(The rest of this is partly a note to myself, partly a recognition of the pain points you experienced in contributing.)

I saw parts of your twitch stream and collected notes where my specification was unclear to remind myself to document and/or provide this information in the future.

  • .meta/hints.md is optional and can be created if it doesn't exist.
  • Hard to find examples of hints in exercises: Caused by a bug we're fixing in #864.
  • Saying diamond :: Char -> Maybe [Text] in the hint twice does seem superfluous. Let's keep it like this for now. We can revisit changing the Data.Text hint into a shorter form for all exercises for which this applies.

As for:

  • "Do I really wanna put configlet in bin/?" I had mixed feelings.
  • "problem-specifications: Path not found": Kudos for assuming this was an Exercism repo.

I should have told you that bin/ensure-readmes-are-updated.sh fetches configlet and clones the problem-specifications repo for you; I forgot to tell you this, because I use configlet directly; the advantage of bin/ensure-readmes-are-updated.sh is that it automatically downloads dependencies if they're not already available.

  • Your quest to explore the CI log: You initially overlooked the error and stopped at the second occurrence of red color, which happened to be a false negative. (This is addressed in #783, but not solved yet.)

@sshine
Copy link
Copy Markdown
Contributor

sshine commented Oct 19, 2019

As a side note unrelated to the completion of this pull request, string-conversions has two interfaces, the other one of which is in the module Data.String.Conversions.Monomorphic in which there's a function toString

toString :: ConvertibleStrings a String => a -> String

that fixates the output type. Some people (such as the author of the string-fromto package) go even further and eliminate the use of type classes here. I've not personally experienced annoyance at type-inference ambiguity when converting strings, but I probably haven't written enough Haskell.

A comparable example you may have run into is when you write x ^ 10 and GHC will say:

$ stack ghc -- -Weverything Pow.hs
...
Pow.hs:5:15: warning: [-Wtype-defaults]
    • Defaulting the following constraints to type ‘Integer’
        (Integral b0) arising from a use of ‘^’ at Pow.hs:5:15-20
        (Num b0) arising from the literal ‘10’ at Pow.hs:5:19-20
    • In the first argument of ‘print’, namely ‘(2 ^ 10)’
      In the expression: print (2 ^ 10)
      In an equation for ‘main’: main = print (2 ^ 10)
  |
5 | main = print (2 ^ 10)
  |               ^^^^^^

because "what 10???"

$ stack ghci
λ> :t 10
10 :: Num p => p

@sshine sshine merged commit 235da42 into exercism:master Oct 21, 2019
@chiroptical chiroptical deleted the diamond_data_text branch October 21, 2019 19:08
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.

3 participants