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

Configurable encoding #153

Merged
merged 2 commits into from
Sep 17, 2018
Merged

Configurable encoding #153

merged 2 commits into from
Sep 17, 2018

Conversation

chpatrick
Copy link
Contributor

By default, libyaml wraps strings at 80 characters with line breaks. This can make it cumbersome to edit long strings in yaml because you have to deal with the inserted newlines, and you can't use your editor's word wrap. Do you think it makes sense to disable this?

@snoyberg
Copy link
Owner

snoyberg commented Sep 9, 2018

I'm not too excited about a cosmetic change like this that others may dislike, given that there's no objective way to measure which approach should be used.

@chpatrick
Copy link
Contributor Author

chpatrick commented Sep 9, 2018 via email

@snoyberg
Copy link
Owner

snoyberg commented Sep 9, 2018

Yeah, I'd be in favor of that. We'd probably need a new set of APIs that alloy passing in an options value containing various options, such as described in #152. encodeWith and encodeFileWith seem like reasonable names. You interested in working on such a PR?

@chpatrick
Copy link
Contributor Author

chpatrick commented Sep 9, 2018 via email

@chpatrick chpatrick changed the title Don't wrap strings at 80 characters. Configurable encoding Sep 9, 2018
@chpatrick
Copy link
Contributor Author

chpatrick commented Sep 9, 2018

I've added encodeWith and encodeFileWith, and also added an option to address #152. However, doesn't this conflict a bit with Data.Yaml.Pretty which also seems to be for customizable encoding?

src/Data/Yaml.hs Show resolved Hide resolved
src/Data/Yaml.hs Outdated
@@ -66,6 +68,10 @@ module Data.Yaml
-- * Classes
, ToJSON (..)
, FromJSON (..)
-- * Custom encoding
, isSpecialString
, EncodeOptions(..)
Copy link
Owner

Choose a reason for hiding this comment

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

To avoid backwards incompatible changes in the future, I'd rather not expose the data constructors here. Instead, I'd like to see something like:

, EncodeOptions
, defaultEncodeOptions
, setEncodeOptionsStringStyle

src/Data/Yaml.hs Outdated
import qualified Text.Libyaml as Y

data EncodeOptions = EncodeOptions
{ encodeOptionsStringStyle :: Text -> ( Tag, Style )
Copy link
Owner

Choose a reason for hiding this comment

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

For whatever identifier gets exported providing support for this, there should be a public comment explaining that missetting this can result in semantically-mismatched YAML documents due to #24. Also, please ensure that any exposed identifiers have a Haddock comment with a @since comment.

@snoyberg
Copy link
Owner

doesn't this conflict a bit with Data.Yaml.Pretty which also seems to be for customizable encoding?

Maybe, but it seems like everyone wants to use the Data.Yaml module.

@chpatrick
Copy link
Contributor Author

Please take another look.

Copy link
Owner

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

Looks good, just a few more minor comments.

ChangeLog.md Outdated Show resolved Hide resolved
src/Data/Yaml.hs Show resolved Hide resolved
src/Data/Yaml.hs Outdated Show resolved Hide resolved
@chpatrick
Copy link
Contributor Author

Done.

Copy link
Owner

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@snoyberg snoyberg merged commit c16c516 into snoyberg:master Sep 17, 2018
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