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

Mapping/Sequence tag output control via FormatOptions #165

Merged
merged 9 commits into from Feb 12, 2019

Conversation

@gmorpheme
Copy link
Contributor

gmorpheme commented Jan 31, 2019

Superseding #164 and as discussed on #164

Tests in YamlSpec show how the various tags behave under each setting.

@@ -668,13 +675,17 @@ parserParseOne' parser = allocaBytes eventSize $ \er -> do
-- @since 0.10.2.0
data FormatOptions = FormatOptions
{ formatOptionsWidth :: Maybe Int
, formatOptionsExplicitMappingTags :: Bool
, formatOptionsExplicitSequenceTags :: Bool

This comment has been minimized.

Copy link
@snoyberg

snoyberg Feb 1, 2019

Owner

Looks good. My only question: would it be more flexible to instead have something like:

data TagType = Explicit | Implicit

formatOptionsTagType :: Event -> TagType

This comment has been minimized.

Copy link
@gmorpheme

gmorpheme Feb 1, 2019

Author Contributor

👍 On it.

I finally like this approach.

There might be even be a sane and safe unification of scalar and collection handling in reach...

@gmorpheme

This comment has been minimized.

Copy link
Contributor Author

gmorpheme commented Feb 1, 2019

OK I've implemented this and, because it seemed such a sensible extension, extended it to cover scalar tags as well, meaning the behaviour is now unified across all tag rendering (with a little wrinkle still for PlainNoTag).

This is obviously a little riskier. I believe the changes I made in Libyaml.hs should result in exactly the same behaviour. All the existing tests pass. However I'm not sure how well covered all the cases were. (See the new tests for all the relevant cases now.)

We could easily remove the changes for scalar tags if you are uncomfortable.

@@ -547,6 +554,18 @@ toEventRaw e f = allocaBytes eventSize $ \er -> do
anchorP
unless (ret == 1) $ throwIO $ ToEventRawException ret
f er
where
implicitColl (EventMappingStart NoTag _ _) = 1

This comment has been minimized.

Copy link
@snoyberg

snoyberg Feb 2, 2019

Owner

This is confusing to me. Why do we need multiple different functions and hard-coding of logic here? Can't we instead leave the necessary logic up to the render function itself?

This comment has been minimized.

Copy link
@gmorpheme

gmorpheme Feb 5, 2019

Author Contributor

Are you saying you'd prefer a single render yes / no function in FormatOptions? If so I agree, but:

  • The NoTag and UriTag "" checks are defensive; things break (by silently disappearing from the output) if you let them through so we should apply these regardless of what the render function might do. (Whether they're applied by pattern matches or by converting to string and checking for null, I'm not particularly fussed. Happy to reinstate the latter - it's more generic.)

  • I think the Plain / Quote distinction is explicit in the libyaml API because the choice of which style is eventually used is made by libyaml itself (if you choose AnyStyle) and may depend on the length of the text etc. So if you want to express for example "pick the best style but show tags for quoted styles but not plain styles" (*), you couldn't do that reliably with a unified render function without writing some brittle code to second guess libyaml's layout rules.

(*) I haven't the first idea why you'd do that. The libyaml C API seems to be designed with it in mind though so I'm wary of discounting it entirely.

This comment has been minimized.

Copy link
@snoyberg

snoyberg Feb 6, 2019

Owner

I'd be fine with ensuring that Implicit is always used for NoTag and UriTag "", but otherwise defer to the user function.

implicitColl (EventSequenceStart NoTag _ _) = 1
implicitColl (EventMappingStart (UriTag "") _ _) = 1
implicitColl (EventSequenceStart (UriTag "") _ _) = 1
implicitColl evt = toEnum $ fromEnum $ formatOptionsRenderCollectionTags opts evt

This comment has been minimized.

Copy link
@snoyberg

snoyberg Feb 2, 2019

Owner

I'd feel better about a more explicit function performing this mapping instead of relying on the Enum instance.

This comment has been minimized.

Copy link
@gmorpheme

gmorpheme Feb 5, 2019

Author Contributor

Yep agreed. Will do. Was just following the approach of Style.


toImplicitParam :: TagRender -> CInt
toImplicitParam Explicit = 0
toImplicitParam _ = 1

This comment has been minimized.

Copy link
@snoyberg

snoyberg Feb 6, 2019

Owner

Minor comment, but I'd prefer to avoid wildcards and be explicit in the constructor names.

@@ -547,6 +554,18 @@ toEventRaw e f = allocaBytes eventSize $ \er -> do
anchorP
unless (ret == 1) $ throwIO $ ToEventRawException ret
f er
where
implicitColl (EventMappingStart NoTag _ _) = 1

This comment has been minimized.

Copy link
@snoyberg

snoyberg Feb 6, 2019

Owner

I'd be fine with ensuring that Implicit is always used for NoTag and UriTag "", but otherwise defer to the user function.

Copy link
Owner

snoyberg left a comment

LGTM!

@snoyberg snoyberg merged commit ac1c995 into snoyberg:master Feb 12, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gmorpheme

This comment has been minimized.

Copy link
Contributor Author

gmorpheme commented Feb 13, 2019

Thanks!

NB. I was unsure of the @since versions, may be worth a check before release. I put in ones which in retrospect would correspond to yaml versions rather than libyaml. And my change log entry went in yaml's ChangeLog instead of libyamls.

@gmorpheme

This comment has been minimized.

Copy link
Contributor Author

gmorpheme commented Feb 13, 2019

Oh I see you've already fixed it :)

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.