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 tag parsing #5382

Merged
merged 1 commit into from
Jan 19, 2016
Merged

Fix tag parsing #5382

merged 1 commit into from
Jan 19, 2016

Conversation

e-dard
Copy link
Contributor

@e-dard e-dard commented Jan 17, 2016

This PR fixes #5380 and #5381.

I've added a new type to the test package, which allows for more robust testing by validating that the tags you specify in your expected case are what you get from the processed point. Prior to this the bugs were being introduced to the expected tags values because one had to use point.Tags() to retrieve them.

Heads up: anywhere that skips a buffer by two places when the current byte is \ (as in if buf[i] == '\\' { i +=2 }) is possibly hiding a bug. I'd be happy to look in more detail at the Points/tags/fields stuff in a couple of weeks when I come on board @jwilder.

@jwilder
Copy link
Contributor

jwilder commented Jan 19, 2016

@jsternberg Can you take a look as well?

@e-dard Looks good. Can you update the changelog as well?

@e-dard e-dard force-pushed the fix-tag-parsing branch 2 times, most recently from 30fc54a to f997f16 Compare January 19, 2016 15:52
@e-dard
Copy link
Contributor Author

e-dard commented Jan 19, 2016

@jwilder updated

// of bytes, starting from i and ending with stop byte, where stop byte
// has not been escaped.
//
// If there are leading spaces, they are skipped.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the extra space at the beginning of this docstring?

@e-dard
Copy link
Contributor Author

e-dard commented Jan 19, 2016

@jsternberg 👍

@jsternberg
Copy link
Contributor

If you can just squash the commits, this looks good to me 👍

@e-dard
Copy link
Contributor Author

e-dard commented Jan 19, 2016

Done

@jsternberg
Copy link
Contributor

👍 and merging since @jwilder gave his +1 too.

jsternberg added a commit that referenced this pull request Jan 19, 2016
@jsternberg jsternberg merged commit c1d6c14 into influxdata:master Jan 19, 2016
@toddboom
Copy link
Contributor

@e-dard nice fixes! let me know if you find yourself bored over the next two weeks. ;)

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.

[0.9.6.1] backslash followed by escaped comma in tag results in wrong value or key
4 participants