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

Pretty, resolve #65 #66

Merged
merged 5 commits into from Aug 13, 2015

Conversation

@phadej
Copy link
Contributor

commented Aug 11, 2015

No description provided.

| otherwise = EventScalar (encodeUtf8 s) StrTag PlainNoTag Nothing

-- | Strings which must be escaped so as not to be treated as non-string scalars.
specialStrings :: HashSet.HashSet Text

This comment has been minimized.

Copy link
@snoyberg

snoyberg Aug 12, 2015

Owner

Shouldn't these two function just be imported from .Internal?

This comment has been minimized.

Copy link
@phadej

phadej Aug 12, 2015

Author Contributor

good catch.

-- | Prettier YAML encoding.
module Data.Yaml.Pretty
( encodePretty
, Config(..)

This comment has been minimized.

Copy link
@snoyberg

snoyberg Aug 12, 2015

Owner

In order to avoid API breakage, I'd prefer:

  • Don't expose the constructor
  • Expose setConfCompare as a new setter function

This is the approach we use in warp and others, and is spelled out in more detail at https://github.com/commercialhaskell/haskelldocumentation/blob/master/content/designing-apis-for-extensibility.md

This comment has been minimized.

Copy link
@phadej

phadej Aug 12, 2015

Author Contributor

I was thinking about this, will make.

@snoyberg

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2015

Overall looks very nice, I just had a few comments.

@phadej

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2015

ping @snoyberg, fixed the issues.

snoyberg added a commit that referenced this pull request Aug 13, 2015

@snoyberg snoyberg merged commit 5fa9d63 into snoyberg:master Aug 13, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@snoyberg

This comment has been minimized.

Copy link
Owner

commented Aug 13, 2015

Thanks!

@phadej phadej deleted the phadej:pretty branch Aug 13, 2015

snoyberg added a commit that referenced this pull request Aug 13, 2015
@phadej

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2015

Thanks for making new release.

Works like a charm: phadej/travis-meta-yaml@0995cb8

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.