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

support anchors and aliases in Data.Yaml.Builder #155

Merged
merged 6 commits into from Sep 27, 2018

Conversation

@dwightguth
Copy link
Contributor

commented Sep 17, 2018

No description provided.

mapping :: [(Text, YamlBuilder)] -> YamlBuilder
mapping pairs = YamlBuilder $ \rest ->
EventMappingStart NoTag AnyMapping Nothing : foldr addPair (EventMappingEnd : rest) pairs
namedMapping :: Maybe String -> [(Text, YamlBuilder)] -> YamlBuilder

This comment has been minimized.

Copy link
@snoyberg

snoyberg Sep 18, 2018

Owner

From an API design standpoint, I would have thought namedMapping would take a String, not a Maybe String, and you would use mapping when no name is available. Also, I would have thought we'd use Text instead of String.

This comment has been minimized.

Copy link
@dwightguth

dwightguth Sep 18, 2018

Author Contributor

We can do that, but then if you want to pass a Maybe to the function you'll have to do an if statement, which is cumbersome compared to the cost of wrapping the string in a Just. So maybe it's better to keep the signature and change the name of the function? Do you have a suggestion?

I will update to change the signature to a Text tomorrow.

@dwightguth

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

I've updated the Maybe to use Maybe Text instead of Maybe String, but I still prefer the signature of a Maybe to a plain Text, because it makes it easier to invoke the function if you have a Maybe as input, while still being pretty easy to construct a Maybe and call the function if you know that you definitely want an anchor. That said, I'm open to alternative naming suggestions for the functions if you have another idea.

@snoyberg

This comment has been minimized.

Copy link
Owner

commented Sep 25, 2018

I don't have another naming suggestion, though I still feel the same as before: the selected naming and type signature is confusing.

@dwightguth

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

I tried renaming the functions to maybeNamedMapping, maybeNamedString, etc. Is that more satisfactory to you?

@snoyberg

This comment has been minimized.

Copy link
Owner

commented Sep 26, 2018

I honestly think it's a bit weird to have the maybe version without the named version, but at this point I'll leave it as-is. Please adjust the PR to have the following:

  • Haddocks with @since comments on all newly exported identifiers
  • ChangeLog entry
  • Minor version bump

@dwightguth dwightguth force-pushed the dwightguth:anchor branch from 477a372 to dd7edac Sep 26, 2018

@dwightguth

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

I went ahead and added the non-maybe version of the named functions based oh your suggestion, and also took into account your other suggested changes. Let me know if there's any other changes you want me to make.

@snoyberg snoyberg merged commit 0a56d96 into snoyberg:master Sep 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.