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 tags and styles for sequences and mappings in Text.Libyaml #141

Merged
merged 10 commits into from Jul 10, 2018

Conversation

@gmorpheme
Copy link
Contributor

commented Jul 10, 2018

This makes tag and style information available on sequences and mappings as well as scalars (at the Text.Libyaml layer - it is not propagated anywhere else).

This would break code using Text.Libyaml because it adds extra fields into the Event data structure.

    EventSequenceStart !Tag !SequenceStyle !Anchor
    EventMappingStart !Tag !MappingStyle !Anchor

...however, I think this is the only sensible way to expose this information. It's definitely an omission at present (although it might very well be an intentional one!). The Tag data type includes MapTag and SeqTag but they'd never be used because tags on mappings and sequences are simply ignored.

gmorpheme added 6 commits Jul 9, 2018
Add style too and tiny refactor
This brings sequence and mapping more into line with scalars
Test we distinguish flow / block styles.
And added separate sequence and mapping style enums.
data SequenceStyle = AnySequence | BlockSequence | FlowSequence
deriving (Show, Eq, Enum, Bounded, Ord, Data, Typeable)

data MappingStyle = AnyMapping | BlockMapping | FlowMapping

This comment has been minimized.

Copy link
@hvr

hvr Jul 10, 2018

Contributor

What's the semantics for AnyMapping?

This comment has been minimized.

Copy link
@gmorpheme

gmorpheme Jul 10, 2018

Author Contributor

See slightly fuller note below. I understand AnyMapping / AnySequence express no preference as to style to emit (block being in most cases the default). The parser should always tell you block or flow.

@hvr
hvr approved these changes Jul 10, 2018
Copy link
Contributor

left a comment

This is a reasonable API change. I've just recently discovered the need to expose the flow/block style in HsYAML myself, see haskell-hvr/HsYAML@e93a4d4

@vedksah vedksah referenced this pull request Jul 10, 2018
@snoyberg
Copy link
Owner

left a comment

Definitely an oversight on my part, thanks for catching this. Can you additionally add a version bump, update the ChangeLog, and add @since comments to the Haddocks of newly added identifiers? It looks like this should be version 0.9.0.

@@ -77,6 +79,12 @@ data Style = Any
| PlainNoTag
deriving (Show, Read, Eq, Enum, Bounded, Ord, Data, Typeable)

data SequenceStyle = AnySequence | BlockSequence | FlowSequence

This comment has been minimized.

Copy link
@snoyberg

snoyberg Jul 10, 2018

Owner

Is there a case where AnySequence will come back from the parser, or is only useful for the emitter?

This comment has been minimized.

Copy link
@gmorpheme

gmorpheme Jul 10, 2018

Author Contributor

AIUI from a reading of the libyaml source, AnySequence and AnyMapping are never produced by the parser (parser.c only references the flow and block constants). They correspond directly to the C enums and the Anys just express no preference as you'd expect. In reality, block is the default barring a few conditions:

e.g.

    if (emitter->flow_level || emitter->canonical
            || event->data.mapping_start.style == YAML_FLOW_MAPPING_STYLE
            || yaml_emitter_check_empty_mapping(emitter)) {
        emitter->state = YAML_EMIT_FLOW_MAPPING_FIRST_KEY_STATE;
    }
    else {
        emitter->state = YAML_EMIT_BLOCK_MAPPING_FIRST_KEY_STATE;
    }

(I admit to some unease about the naming I've chosen here, particularly given this is an API change. Happy to accept alternative suggestions!)

stack.yaml Outdated
@@ -3,3 +3,6 @@ resolver: lts-11.10
flags:
yaml:
no-examples: false

docker:
enable: true

This comment has been minimized.

Copy link
@snoyberg

snoyberg Jul 10, 2018

Owner

Please remove this, Docker should not be the default for the repo.

This comment has been minimized.

Copy link
@gmorpheme

gmorpheme Jul 10, 2018

Author Contributor

Oops. Accidental, will do.

@@ -63,6 +63,12 @@ spec = do
it "count scalars with anchor" caseCountScalarsWithAnchor
it "count sequences with anchor" caseCountSequencesWithAnchor
it "count mappings with anchor" caseCountMappingsWithAnchor
it "count sequences with custom tag" caseCountSequenceTags

This comment has been minimized.

Copy link
@snoyberg

snoyberg Jul 10, 2018

Owner

Three cheers for new tests!

@gmorpheme

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

I've popped a @since on SequenceStyle and MappingStyle, bumped the package.yaml version and added a changelog entry.

bsToTag <$>
if ytag_len' == 0
then return Data.ByteString.empty
else packCStringLen (ytag', ytag_len')

This comment has been minimized.

Copy link
@snoyberg

snoyberg Jul 10, 2018

Owner

Given that under the surface the C code is just using strlen anyway to look for a null-terminating byte, is there a reason to use packCStringLen instead of packCString? It seems like the latter would allow us to simplify the changeset a bit.

@@ -77,6 +79,14 @@ data Style = Any
| PlainNoTag
deriving (Show, Read, Eq, Enum, Bounded, Ord, Data, Typeable)

-- @since 0.9.0

This comment has been minimized.

Copy link
@snoyberg

snoyberg Jul 10, 2018

Owner

This should be -- | @since 0.9.0 to make Haddock recognize it. Bonus points would be a short sentence explaining what the type is used for.

tagbs <-
if ytag_len' == 0
then return Data.ByteString.empty
else packCStringLen (ytag', ytag_len')

This comment has been minimized.

Copy link
@snoyberg

snoyberg Jul 10, 2018

Owner

Heh, doh, now I realize why you have it set up this way with the len, I did it that way in the first place. I think my claim that we can use packCString instead still applies, but your call on whether to include that now or if I try it separately after this gets merged.

This comment has been minimized.

Copy link
@gmorpheme

gmorpheme Jul 10, 2018

Author Contributor

Ha. Yep - was trying to make minimal changes here but I didn't understand why the len approach was there. I'll kill it.

There's also some inconsistency between checking null ptrs in the helper C and in the Haskell but it kinda makes sense to unify null and "" for the case checking in the tag case.

@gmorpheme

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

Removed the len stuff for tag handling - should be functionally identical. We'd get into trouble anyway if a tag contained a null but that's illegal (at least in YAML 1.2 - http://yaml.org/spec/1.2/spec.html#ns-uri-char).

Haven't touched the len stuff with the value. Felt like a step too far for this change.

@gmorpheme

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

Oops - and have a failing test. Hold off...

@gmorpheme

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

Fixed.

@snoyberg
Copy link
Owner

left a comment

This is great stuff, thank you!

| EventMappingEnd
deriving (Show, Eq)

-- | Style for scalars - e.g. quoted / folded

This comment has been minimized.

Copy link
@snoyberg

snoyberg Jul 10, 2018

Owner

Thank you for cleanups like this 👍

This comment has been minimized.

Copy link
@gmorpheme

gmorpheme Jul 10, 2018

Author Contributor

Well, thank you for the whole library. 👍

@snoyberg snoyberg merged commit f01e81c into snoyberg:master Jul 10, 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
3 participants
You can’t perform that action at this time.