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

Add Json stream wrappers #60

Merged
merged 1 commit into from
Oct 1, 2020
Merged

Add Json stream wrappers #60

merged 1 commit into from
Oct 1, 2020

Conversation

satabin
Copy link
Member

@satabin satabin commented Sep 30, 2020

Closes #48

@satabin satabin added enhancement New feature or request json labels Sep 30, 2020
@satabin satabin added this to the 0.8.0 milestone Sep 30, 2020
* The resulting token stream is a valid single JSON object stream, iff the original
* stream is a valid stream of JSON values.
*/
def asArray[F[_], Json](at: String, in: Map[String, Json] = Map.empty[String, Json], mapFirst: Boolean = true)(
Copy link
Collaborator

Choose a reason for hiding this comment

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

asArray makes me think of [e1, e2, ...], not {"a":true, "s":[e1, e2, ...]} and I believe both have their use cases (though the first has a way simpler implementation, a pipe here could be user-friendly). I'm also thinking of deeply nested structures which now you can build by using several of these pipe, but a selector-based method like inject[F[_]](wrapper: Stream[F, Token], at: Selector, early: Boolean = true): Pipe[F, Token, Token] might be more user-friendly.

All these can be implemented in future PRs, I just want mention so that you can reconsider naming. For that, I'll approve, but not merge yet, do you as think. Definitely a very nice PR with awesome documentation, really beautiful!

Copy link
Member Author

@satabin satabin Oct 1, 2020

Choose a reason for hiding this comment

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

Actually the name is wrap.asArray(at = "key") which is quite explicit I think. I miss the SmallTalk syntax for this kind of DSL. I was thinking about adding a asTopLevelArray as well, I can do it. Potentially rename asArray into something like asArrayInObject but I like it less.

I though about the selector and rejected it because it's hard to make it work properly in a streaming fashion and a user friendly way. Think about how you would put the stream into

{
  "a": {
    "b": {
      "c": true
   },
  "d": 1
}

at selector .a.b.c.d with early = true.

Arbitrary depth is just only corner cases, or then putting constraints into the doc and hoping users will keep it in mind at all time.

Copy link
Member Author

Choose a reason for hiding this comment

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

And on top of that, selectors allow for .a.[].b, how to interpret it? We would need JSON Path instead of selector here (the later are intended to be used to enumerate and select potentially several values, not identifying a unique one). This is something I thought about as well, but I think it's outside of the scope of this PR.

@ybasket ybasket merged commit 9562509 into master Oct 1, 2020
@ybasket ybasket deleted the 48-json-wrappers branch October 1, 2020 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request json
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Json Stream Wrappers
2 participants