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

prefer String instance of ToYaml over [Char] #186

Merged
merged 2 commits into from May 7, 2020

Conversation

bergey
Copy link
Contributor

@bergey bergey commented Apr 11, 2020

No description provided.

@snoyberg
Copy link
Owner

I'd be very nervous about including OVERLAPPING like this and would like to see more motivation before including it.

@bergey
Copy link
Contributor Author

bergey commented Apr 13, 2020

Sorry for the lack of explanation. The current String instance overlaps with the general [a] instance. As I understand it, without some guidance, GHC won't pick between them; my attempts to call toYaml on String do not compile. I think OVERLAPPING is less intrusive than OVERLAPPABLE on the [a] instance.

@snoyberg
Copy link
Owner

The String instance was added in bde18b5. Maybe @robbiemcmichael can weigh in here?

@robbiemcmichael
Copy link
Contributor

robbiemcmichael commented Apr 14, 2020

This one's my fault for not adding a test that called toYaml on a String.

I was under the impression that the constraint on

instance ToYaml a => ToYaml [a] where
    toYaml = array . map toYaml

would mean that the instances aren't overlapping since there's no ToYaml instance for Char, but that doesn't seem to be the case.

I had a look at how the list instance works for Show. They define an extra function showList with a default defined in the type class which gets overridden with a special definition for the Char instance. That trick looks like it might work here but it would require defining an instance of ToYaml for Char. Although Char doesn't really have a corresponding YAML type, we could just choose to convert it to a YAML string containing just a single character.

I don't know if that mess is more or less desirable than using OVERLAPPING so I'd probably rather someone else made that call.

@bergey
Copy link
Contributor Author

bergey commented Apr 14, 2020

I think adding a list method to ToYaml would also meet my needs.

My understanding is that GHC looks for an instance that matches right of the =>, and if it finds one, searches for the constraints from the left of the =>. I think it never backtracks.

@snoyberg
Copy link
Owner

I don't use this particular API myself. But my leaning is to avoid usage of String at all, as well as the associated lists [(a, b)]. That said: I'd be fine with OVERLAPPING. Can both of you check that this PR actually works as you'd expect?

@bergey
Copy link
Contributor Author

bergey commented May 5, 2020

Below are a few tests that I believe cover the instance resolution. I have a somewhat larger project that uses YamlBuilder to output hpack package.yaml files, where it is also behaving as expected.

import Data.Yaml.Builder
λ BS.putStrLn $ toByteString ([1..4] :: [Int])
- 1
- 2
- 3
- 4

λ BS.putStrLn $ toByteString "a single string"
a single string

λ BS.putStrLn $ toByteString (words "several strings in a list")
- several
- strings
- in
- a
- list

@snoyberg
Copy link
Owner

snoyberg commented May 5, 2020

OK, I'm fine with including this change as-is. Would you mind adding a ChangeLog entry and minor version bump?

@snoyberg snoyberg merged commit bacf680 into snoyberg:master May 7, 2020
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.

None yet

3 participants