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

Support for parsing multiple documents #369

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

coreyoconnor
Copy link

@coreyoconnor coreyoconnor commented Dec 23, 2024

This adds parseAllYamls, parseYaml, and the asMany extension method to the yaml package object.

  • parseAllYamls either fails or returns a list of all yaml documents (Node) contained in the string content.
  • parseYaml is an explicit form of the is the single document parsing provided by .asNode.
  • String extension method asMany[T] - decodes string of multiple documents into a list of T

To implement this, a fairly small addition was made to Composer: multipleFromEvents. Which uses composeNode to parse Nodes from the events so long as there are tail events returned.

This also exposes the pos method of Node. This is useful when writing custom decoders for nicer error messages.

This adds `parseYAML` and `parseAllYAMLs` to the `yaml` package object.

The first is the single document parsing provided by `.asNode`. The
second parses a sequence of documents.

To implement this, a fairly small addition was made to `Composer`:
`multipleFromEvents`. Which uses `composeNode` to parse `Node`s from the
events so long as there are `tail` events returned.

This also exposes the `pos` method of `Node`. This is useful when
writing custom decoders for nicer error messages.
@tgodzik
Copy link
Member

tgodzik commented Dec 30, 2024

@lbialy could you take a quick look?

Copy link
Contributor

@lbialy lbialy left a comment

Choose a reason for hiding this comment

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

Everything looks fine, just one nitpick that I have is that the convention is to have Yaml and not YAML so please rename the parseYAML to parseYaml and parseAllYAMLs to parseAllYamls.

Also a comprehensive test (from String to List of some T and back) would be nice.

@coreyoconnor
Copy link
Author

Thanks for the review! Made the style changes and added a minimal test.

@lbialy
Copy link
Contributor

lbialy commented Jan 17, 2025

Could you please rebase? I think the missing commits that touched sbt conf might be causing CI to fail (no idea why though :/ ).

Also, wouldn't it also make sense to add .asMany[T] combinator in this PR?

@coreyoconnor
Copy link
Author

coreyoconnor commented Jan 17, 2025 via email

@coreyoconnor
Copy link
Author

@lbialy Done with the latest set of changes. Given the term asMany uses "many" for plural I changed the parseAllYamls to parseManyYamls

There is also style change I like, but feel free to request me to undo: Have the String extension methods forwards to top level methods: decodeYaml, decodeManyYamls etc.

@coreyoconnor
Copy link
Author

fixing...

@coreyoconnor
Copy link
Author

what a strange one. The implicit string conversion in base suite conflicted with the one in the package object. Yet the compile was fine with this - until I did a clean recompile.

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.

3 participants