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

Features to preserve Flow Mapping and Flow sequence #18

Merged
merged 3 commits into from
Jun 13, 2019

Conversation

vijayphoenix
Copy link
Collaborator

  1. These changes help to preserve Flow style Mapping and Flow style Sequence information
  2. Fix bugs that occurred after a round-trip
  3. Minor code improvements
  4. These changes will not become a source for some other errors/problems.

Stats on YAML-test suite

done -- passed: 316 (ev: 32, ev+json: 128, ev+json+yaml: 85, err: 71) / failed: 2 (err: 2, ev:0, json:0, yaml:0, ok:0)

It was also tested on some additional examples which can be found here

Copy link
Collaborator

@hvr hvr left a comment

Choose a reason for hiding this comment

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

here's a first code-review iteration feedback :-)

src/Data/YAML/Event/Writer.hs Outdated Show resolved Hide resolved
src/Data/YAML/Event/Writer.hs Outdated Show resolved Hide resolved
src/Data/YAML/Event/Writer.hs Outdated Show resolved Hide resolved
src/Data/YAML/Event/Writer.hs Outdated Show resolved Hide resolved
src/Data/YAML/Event/Writer.hs Outdated Show resolved Hide resolved
@hvr
Copy link
Collaborator

hvr commented Jun 13, 2019

There's one issue though with this change:

-done -- passed: 316 (ev: 32, ev+json: 93, ev+json+yaml: 120, err: 71) / failed: 2 (err: 2, ev:0, json:0, yaml:0, ok:0)
+done -- passed: 316 (ev: 32, ev+json: 128, ev+json+yaml: 85, err: 71) / failed: 2 (err: 2, ev:0, json:0, yaml:0, ok:0)

i.e. 35 testcases seem to have soft-regressed to not match their YAML reference output anymore; mostly because now we emit the less idiomatic flow-style instead of block-style more often

you'll see this when you diff the output of run-tml before/after your PR

this isn't necessarily a problem with your code but we might need to tweak the run-tml code to have the out-yaml data be normalised like before

@hvr hvr merged commit a213dff into haskell-hvr:0.2 Jun 13, 2019
@vijayphoenix vijayphoenix added the enhancement New feature or request label Jun 15, 2019
@vijayphoenix vijayphoenix self-assigned this Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants