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

influxdb/src/query: Send uints as unsigned integers on the line protocol (u suffix instead of i) #113

Merged
merged 1 commit into from
Aug 27, 2022

Conversation

mcronce
Copy link
Contributor

@mcronce mcronce commented Aug 24, 2022

Description

Currently, we serialize u8/u16/u32/u64 fields as signed integers in the line protocol (e.g. 82i). This changes them to send as unsigned integers (e.g. 82u)

It might make sense for this be opt-in behavior, or at the very least a breaking change as far as semver is concerned (e.g. v0.6.0); unsigned integer support wasn't merged into influxdb until 1.4.0, so users with sufficiently old versions of influxdb could have a bad experience.

Quick note - I did run clippy and it fails on a few things, but nothing I changed. I can include fixes if desired, but I didn't want to tack on unrelated changes that might be considered out of scope - let me know!

Checklist

  • Formatted code using cargo fmt --all
  • Linted code using clippy
    • with reqwest feature: cargo clippy --manifest-path influxdb/Cargo.toml --all-targets --no-default-features --features use-serde,derive,reqwest-client -- -D warnings
    • with surf feature: cargo clippy --manifest-path influxdb/Cargo.toml --all-targets --no-default-features --features use-serde,derive,hyper-client -- -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

Sorry, something went wrong.

@msrd0
Copy link
Collaborator

msrd0 commented Aug 24, 2022

Quick note - I did run clippy and it fails on a few things, but nothing I changed. I can include fixes if desired, but I didn't want to tack on unrelated changes that might be considered out of scope - let me know!

Don't mind them, I believe we should pin the clippy version so that newly introduced lints don't fail PRs with unrelated changes (see #114).

@Empty2k12
Copy link
Collaborator

I have merged #114 to main, you can rebase and run the CI.

Verified

This commit was signed with the committer’s verified signature.
gregberge Greg Bergé
…fix instead of `i`)
@mcronce
Copy link
Contributor Author

mcronce commented Aug 26, 2022

@Empty2k12 done, pushed up the rebase

@mcronce
Copy link
Contributor Author

mcronce commented Aug 26, 2022

FWIW, I do still get these on nightly:

error: deref which would be done by auto-deref
  --> influxdb/src/query/line_proto_term.rs:26:51
   |
26 |             Measurement(x) => Self::escape_any(x, &*COMMAS_SPACES),
   |                                                   ^^^^^^^^^^^^^^^ help: try this: `&COMMAS_SPACES`
   |
   = note: `-D clippy::explicit-auto-deref` implied by `-D warnings`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#explicit_auto_deref

error: deref which would be done by auto-deref
  --> influxdb/src/query/line_proto_term.rs:27:60
   |
27 |             TagKey(x) | FieldKey(x) => Self::escape_any(x, &*COMMAS_SPACES_EQUALS),
   |                                                            ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&COMMAS_SPACES_EQUALS`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#explicit_auto_deref

error: deref which would be done by auto-deref
  --> influxdb/src/query/line_proto_term.rs:47:63
   |
47 |             Text(v) => format!(r#""{}""#, Self::escape_any(v, &*QUOTES_SLASHES)),
   |                                                               ^^^^^^^^^^^^^^^^ help: try this: `&QUOTES_SLASHES`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#explicit_auto_deref

error: deref which would be done by auto-deref
  --> influxdb/src/query/line_proto_term.rs:65:44
   |
65 |             Text(v) => Self::escape_any(v, &*SLASHES),
   |                                            ^^^^^^^^^ help: try this: `&SLASHES`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#explicit_auto_deref

error: could not compile `influxdb` due to 4 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `influxdb` due to 4 previous errors

Stable looks clean, though :)

@Empty2k12 Empty2k12 merged commit a3e4a2d into influxdb-rs:main Aug 27, 2022
@Empty2k12
Copy link
Collaborator

Thanks!

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.

None yet

3 participants