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

Allow GROUP BY queries by providing deserialize_next_tagged to deserialize the group fields #69

Merged
merged 1 commit into from
Oct 3, 2020

Conversation

SafariMonkey
Copy link
Contributor

Description

This change adds deserialisation for the metadata of a GROUP BY using the method deserialize_next_tagged, which takes a TAG parameter (so called because that's what the InfluxDB response calls it) which is deserialized from the metadata. Because Series has a custom deserializer, this means TaggedSeries also has to. It's pretty copy paste, with minimal code changes, and leverages the existing Series value deserialisation. I originally tried to abstract over having tags and not but it ended up being way hairier and more error prone, so I thought copying ~100 lines of boilerplate is probably fine.

I've tested this on my code and it works fine. I also added a test and that passes.

Side note, is the #[doc(hidden)] something that can be removed? The user has to use that struct to access the series.

Checklist

  • Formatted code using cargo fmt --all
  • Linted code using clippy cargo clippy --all-targets --all-features -- -D warnings
  • Updated README.md using cargo readme -r influxdb -t ../README.tpl > README.md
  • Reviewed the diff. Did you leave any print statements or unnecessary comments?
  • Any unfinished work that warrants a separate issue captured in an issue with a TODO code comment

@Empty2k12
Copy link
Collaborator

Empty2k12 commented Oct 3, 2020

Hey @SafariMonkey, sorry for my absence. I tested this PR and it's working beautifully. Thanks for the work!

Side note, is the #[doc(hidden)] something that can be removed? The user has to use that struct to access the series.

In the past, it was not needed, so that's why the #[doc(hidden)] was added. If it's public, maybe a better name for TaggedReturn can be chosen, or do you think it's okay to keep it like that?

@cryptoquick
Copy link

A rose by any other name would still smell as sweet. I suggest merging this, and if there's additional aesthetic clarity, it can come in via another pull request.

@SafariMonkey
Copy link
Contributor Author

I'm fine with @cryptoquick's suggestion. If you want to change the name feel free, but I'm happy enough with it as is. I already gave it a once over before submitting it.

@Empty2k12 Empty2k12 merged commit 7e66d2f into influxdb-rs:master Oct 3, 2020
@Empty2k12
Copy link
Collaborator

Thanks for your second awesome contribution!

@cryptoquick
Copy link

@SafariMonkey yes, thanks for working on this even though it's no longer core to your project. And thank you, @Empty2k12 for being such a responsive maintainer, even over the weekend! Rust is a healthier community thanks to both of your efforts.

@Empty2k12
Copy link
Collaborator

I will release a build on crates.io today or tomorrow. Thanks again.

@SafariMonkey
Copy link
Contributor Author

@cryptoquick As a slight correction, I was only saying before that writing to InfluxDB wouldn't be able to be handled by this library due to its quoting - the reading is still this library. It's only a prototype I'm putting together during some spare moments, hence the long silence last month.

I'm happy that these PRs can help the project, and I will continue to submit them should I run into areas that I think could use improvement.

I'd also like to echo the appreciation for your work in reviewing these changes and your responsiveness, @Empty2k12. And it's nice to have a PR so well received :)

@Empty2k12
Copy link
Collaborator

@SafariMonkey @cryptoquick Sorry for the long wait, but the v0.2.0 version is now live on crates.io.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants