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

Stream IOs #18

Merged
merged 12 commits into from
Jul 25, 2019
Merged

Stream IOs #18

merged 12 commits into from
Jul 25, 2019

Conversation

Blacksmoke16
Copy link
Owner

@Blacksmoke16 Blacksmoke16 commented Jul 19, 2019

Description

Resolves #14
Resolves #15
Resolves #17
Resolves #19

  • Refactors much of oq to more easily support additional formats.
    • .deserialize for Xml converter will follow later.
  • Follows up on Refactors to optimize IO #6 to extend the streaming of data to YAML and XML in addition to JSON.
    • This makes oq much much more memory efficient as nothing is loaded into memory.

Big shoutout to @asterite for the gist that made this PR possible.

TODO

  • Support #text keys again
  • Add specs for each format
  • Make sure current jq features still work

Adds -V --version cli command
Add ability to change default array element name
Add oq and jq version to help message
@Blacksmoke16 Blacksmoke16 marked this pull request as ready for review July 24, 2019 12:14
@Blacksmoke16
Copy link
Owner Author

@asterite would you mind giving this a review if you get a minute?

@asterite
Copy link

@Blacksmoke16 Sure! I'll do it later today.

Copy link

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Looks good! Just added one suggestion.

Eventually if I have time I can try to see if I can write code to handle YAML aliases and anchors. Sounds like something fun and challenging that I would enjoy, but it all depends on whether I'll have time to do it.


private def self.parse_args(args : NamedTuple) : Tuple(String, Bool, String, String)
{
args["indent"],

Choose a reason for hiding this comment

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

Oh, wow, I didn't remember you could access a named tuple with a string, I though only symbol was valid.

@@ -0,0 +1,88 @@
module OQ::Converters::Xml
@@at_root : Bool = true

Choose a reason for hiding this comment

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

This is a bit of a smell, it means you can't call methods of this module concurrently. Is there a way to pass this at_root value to all methods that need it so it's not shared at all?

Copy link
Owner Author

@Blacksmoke16 Blacksmoke16 Jul 25, 2019

Choose a reason for hiding this comment

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

Yea I could probably just pass it between the methods versus making it a class var.

EDIT: Or maybe it would be worth making these structs and using an initializer. Which could also be useful for setting ivars for the arg values?

Choose a reason for hiding this comment

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

Oh, I edited my comment and some part was gone. I forgot to say that this is probably a minor issue because this is a one-shot app meaning that you won't be running this concurrently, but it's still a good design pattern to avoid sharing stuff.

I thought about making it a class/struct but you'd still have the same problem that if someone uses that instance concurrently then the @at_root instance var would cause conflicts. I think there's no other way around passing it between methods. And instead of passing a lot of args you can consider creating a class holding these shared things and passing that instead.

But again, this is probably not needed for this kind of app 😊

@Blacksmoke16 Blacksmoke16 merged commit c8814e7 into master Jul 25, 2019
@Blacksmoke16 Blacksmoke16 deleted the streaming branch July 25, 2019 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants