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

Expose function for encoding YAML events #179

Closed
wants to merge 1 commit into from

Conversation

robbiemcmichael
Copy link
Contributor

I'd like to expose a function for encoding raw YAML events for a project I'm working on. I'm not entirely sure it should be exposed from this module, but happy to refactor if you'd prefer something else.

. (:) EventDocumentStart
$ objToEvents' o
$ objToEvents opts o ++
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather not have to lose the difference list approach here. It's not a major performance difference, but it's nice to keep.

[ EventDocumentEnd
, EventStreamEnd
]

-- | Encode a value into YAML 'Event's
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a @since Haddock comment, as well as a minor version bump and ChangeLog entry.

@robbiemcmichael
Copy link
Contributor Author

@snoyberg Thanks for the review, updated as requested.

[ EventDocumentEnd
, EventStreamEnd
]

-- | Encode a value as YAML 'Event's.
Copy link
Owner

Choose a reason for hiding this comment

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

Just one more request on the docs: it's a pretty niche use case to need this function. I'd like to see either (or, ideally, both) of:

  1. Including some explanation in the docs here as to why someone may want this
  2. Instead of including it in the export list with the more commonly used functions, include it in a separate section for less-commonly-used functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it would be better for objToEvents (and possibly objToStream) to be moved to the Data.Yaml.Internal module and exported from there since they're both quite low-level.

There also seems to be another function pretty which is doing a very similar job, but this function is also not exported. Unless I'm mistaken, this version can only print strings containing new lines using single quotes so oddly enough it's less pretty than objToEvents.

Is there a reason for both to exist or would you prefer I had a go at refactoring them to use a common function which could be exported from Data.Yaml.Internal?

Copy link
Owner

Choose a reason for hiding this comment

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

Exposing from .Internal and refactoring sounds great, assuming no user-visible behavior changes.

@robbiemcmichael
Copy link
Contributor Author

Closing this in favour of a new pull request which refactors the code to reduce some of the duplication between encode and encodePretty.

robbiemcmichael added a commit to robbiemcmichael/yaml that referenced this pull request Oct 29, 2019
Exposes `encodeObject` from `Data.Yaml.Internal` to reduce some of the
code duplication between the `encode` and `encodePretty` functions.

The output of `encodePretty` has changed as a result:
- Multiline strings now use `Literal` style instead of `SingleQuoted`.
- Special keys are now quoted in maps (resolves snoyberg#179).
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

2 participants