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

fix: handle string output with streams #36

Merged
merged 7 commits into from
Apr 16, 2023

Conversation

dainiusjocas
Copy link
Owner

@dainiusjocas dainiusjocas commented Apr 15, 2023

This PR is a partial fix/hack for the cases when the jq script produces multiple outputs, e.g.

$ jq '.[]' <<<'[1,2,3]'

The problem was that this lib returned only the last emitted JSON entity.

In order not to introduce severe breaking changes, the following behavior is introduced:

  1. For cases when the output is a string, multiple outputs are joined by a new line.
  2. When JsonNode is expected the output is always ArrayNode which extends both JsonNode and Iterable.

With these changes, the types returned are the same as previously but values changed significantly in some cases (so the breaking changes are masquerading as "fixes"). The overall behavior ends up being "tricky".

BREAKING CHANGES!

What's next:

@dainiusjocas dainiusjocas marked this pull request as ready for review April 15, 2023 23:15
Copy link
Contributor

@charles-dyfis-net charles-dyfis-net left a comment

Choose a reason for hiding this comment

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

Using newline separators could be messy if we supported a raw-output mode, since that use case would create ambiguity between newlines within content and newlines separating content; but so long as that's not intended, this seems perfectly reasonable. (And even if that is a direction this project wanted to go in, users to whom it mattered could request json-node output)

@dainiusjocas dainiusjocas force-pushed the refactor/string-output-to-use-joined-stream branch from 89ce63d to 145b820 Compare April 16, 2023 20:04
@dainiusjocas
Copy link
Owner Author

Using newline separators could be messy if we supported a raw-output mode, since that use case would create ambiguity between newlines within content and newlines separating content; but so long as that's not intended, this seems perfectly reasonable. (And even if that is a direction this project wanted to go in, users to whom it mattered could request json-node output)

I agree that using new lines is messy, but for now IMO, it is better than "just return the last emit" which is clearly wrong. As for the JsonNode return case, nothing really changes. Legacy.

In #39 I'll introduce an API that will be simpler and would return Iterable as it is done in gojq. And with #40 I believe most of the normal JQ functionality could be properly implemented with a sequence of configurable transducers.

@dainiusjocas dainiusjocas merged commit 11c38a4 into main Apr 16, 2023
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.

2 participants