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 parsing application/json-seq #110

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

spbnick
Copy link
Contributor

@spbnick spbnick commented May 30, 2024

Support parsing RS-separated streams, as per RFC 7464.

Hi Michael, here are some (possibly) useful changes separated from my stream parsing support, freshly rebased.

Would be glad to hear your comments and fix it up as necessary!

Add the `slurp` argument to all `_Program.input*()` methods, and pass it
through, so it's possible to supply it with every method. But most
importantly the `_Program.input()`, which also accepts `text`.
Let Python give us the length of the "bytes" it already knows, instead
of doing an strlen(). This improves performance a bit.
@spbnick
Copy link
Contributor Author

spbnick commented May 30, 2024

CI failed seemingly due to pypi.org certificate verification issues.

@spbnick
Copy link
Contributor Author

spbnick commented May 31, 2024

Looks like the only broken test job is Build / tests (ubuntu-20.04, 3.5, false), and that's due to SSL.

@mwilliamson
Copy link
Owner

Just to check I've understood, it seems like there are two main changes here:

  1. Using a JSON text sequence as an input
  2. Outputting a JSON text sequence

Specifically, when passing in seq=True to the various input methods, any input text will be treated as a JSON text sequence, and any text output from .text() will be formatted as a JSON text sequence.

Have I understood that correctly?

@spbnick
Copy link
Contributor Author

spbnick commented Jun 2, 2024

Just to check I've understood, it seems like there are two main changes here:

  1. Using a JSON text sequence as an input
  2. Outputting a JSON text sequence

Specifically, when passing in seq=True to the various input methods, any input text will be treated as a JSON text sequence, and any text output from .text() will be formatted as a JSON text sequence.

Have I understood that correctly?

Yes. I did it this way, because the jq tool does it this way, when you supply the --seq option. In my own code I separate these. I'm up for redoing this the way you'd prefer it.

Thanks for the review!

@mwilliamson
Copy link
Owner

I think you could make an argument for a seq argument applying to both the input and output, since, as you say, that matches jq, but given the Python bindings have a explicit input methods, you might reasonably expect that only to apply to the input.

Given the existing API, I think the least ambiguous approach might be to leave the existing methods as they are, and instead add two new methods, input_text_sequence() (for the input) and text_sequence() (for the output). What do you think?

Support parsing RS-separated streams, as per RFC 7464.
@spbnick
Copy link
Contributor Author

spbnick commented Jun 5, 2024

Given the existing API, I think the least ambiguous approach might be to leave the existing methods as they are, and instead add two new methods, input_text_sequence() (for the input) and text_sequence() (for the output). What do you think?

That works for me, updated!

@spbnick
Copy link
Contributor Author

spbnick commented Jun 5, 2024

There's still SSL certificate verification problem in Build / tests (ubuntu-20.04, 3.5, false) (pull_request). I just removed the 3.5 from the matrix in my fork and the rest worked.

@mwilliamson
Copy link
Owner

I've just dropped support for Python 3.5, seeing as we don't build wheels for it anyway and usage seems low.

@mwilliamson
Copy link
Owner

I think this is good to go with the new methods. I'd prefer not to add slurp to the other input methods though: I think they make less sense outside of the context of text input (since you can just wrap the value in a list), and I think are unrelated to parsing text sequences.

There's also missing tests for outputting text sequences, but I'm happy to add those tests once this is merged.

@spbnick
Copy link
Contributor Author

spbnick commented Jun 13, 2024

Sorry, I'm on a conference trip and don't have time to continue this right now. I'll get back to this the week after next one.

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