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 improper quoting on tag values when the value was text #64

Merged
merged 2 commits into from
Aug 26, 2020

Conversation

sparky8251
Copy link
Contributor

@sparky8251 sparky8251 commented Aug 20, 2020

Description

Fixes #60

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 > 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

@sparky8251
Copy link
Contributor Author

sparky8251 commented Aug 20, 2020

Looks like there were clippy lint issues as part of master and I'm unsure if I should fix them. I also tried tests but those seem broken and unable to run as well?

It's likely this change breaks a test though so if its required for merging, just going to need help getting it working so I can fix them (or at least a name of the broken test since the fix should be trivial).

@Empty2k12
Copy link
Collaborator

Thanks for fixing the issue 👍 🚀

There should have been a CI run showing which tests fail. I'll check what that didn't trigger.

@Empty2k12
Copy link
Collaborator

@sparky8251 I have added a new CI pipeline based on Github Actions and also fixed the clippy issues.

Sadly I do not have maintainer write permissions to your fork, so it would be nice if you could rebase to the latest master and have CI tell us which tests are incorrect.

@sparky8251
Copy link
Contributor Author

sparky8251 commented Aug 26, 2020

Ok, All the clippy lints and tests pass now.

The query::write_query::tests::test_escaping test was especially brutal and I'd spend 90% of the review time on that test as while I've got it passing, I'm only 99.9% certain I retained the initial intent behind the test given all the rando backslashes and double quotes in so many places.

Squashed down all my test attempts so its just 2 discrete commits. If you want 1, let me know.

@Empty2k12
Copy link
Collaborator

Taking a look now! Thanks for rebasing!

The query::write_query::tests::test_escaping test was especially brutal and I'd spend 90% of the review time on that test as while I've got it passing, I'm only 99.9% certain I retained the initial intent behind the test given all the rando backslashes and double quotes in so many places.

Honestly InfluxDb line protocol is very painful. As this PR uncovered, my initial implementation is not correct even though I based the escaping parts and tests off of the official Go package.

@Empty2k12
Copy link
Collaborator

From what I can tell, the new behavior looks correct. All types of escaped tested in the go sdk are covered.

Thanks for creating this PR and helping make this crate better.

@Empty2k12 Empty2k12 merged commit f40c2ec into influxdb-rs:master Aug 26, 2020
@sparky8251
Copy link
Contributor Author

No problem! Using this crate to pull sensor data from embedded devices and this was something that bit me. Ended up making it incompatible with other libraries and methods of inputting data into influxdb so I had to fix it :)

@Empty2k12
Copy link
Collaborator

I think this change will be pushed to crates.io when #62 is merged. Until then, you reference this repo directly by Git.

If you come across further problems, feel free to open issues or pull requests addressing them. All improvements appreciated.

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.

tags without quotation marks
2 participants