Skip to content

Conversation

@leonardodanelutti
Copy link
Contributor

No description provided.

@leonardodanelutti leonardodanelutti changed the title Conditional compilation for unix tragets. Conditional compilation for unix tragets and implementation of timestamps. Mar 18, 2023
@maxmindlin
Copy link
Owner

Thanks for the contribution! I agree that the cfg compilation check was an oversight and should be added. Please separate out that from the timestamp addition into different PRs. I'm not yet convinced timestamps are worth adding and need time to consider your implementation.

@leonardodanelutti
Copy link
Contributor Author

Thank you for your feedback! Should I close this PR and open two new ones?

@maxmindlin
Copy link
Owner

Thank you for your feedback! Should I close this PR and open two new ones?

whichever way is easier for you, I dont have a preference. As long as its one topic per PR

@leonardodanelutti leonardodanelutti changed the title Conditional compilation for unix tragets and implementation of timestamps. Implementation of timestamps. Mar 19, 2023
@maxmindlin
Copy link
Owner

Closes #10

@maxmindlin
Copy link
Owner

what happens when you have more than one timestamp struct attr? Ex.,

struct Foo {
    a: String,
    #[telegraf(timestamp)]
    t1: u32,
    #[telegraf(timestamp)]
    t2: u32,
}

obviously thats not intended to be used that way, but curious what it does and what unintended behavior there might be.

@leonardodanelutti
Copy link
Contributor Author

what happens when you have more than one timestamp struct attr?

Only the last one specified will be used.

Co-authored-by: maxmindlin <35264981+maxmindlin@users.noreply.github.com>
Comment on lines +51 to +56
#[allow(unused_mut)]
let mut timestamp: Option<u64> = None;
$(
timestamp = timestamp.or(Some($ts));
)?

Copy link
Owner

Choose a reason for hiding this comment

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

I think this could be done differently, but honestly I'm so rusty at macros that I cant remember how. We should be able to just use $ts but the macro thinks its repeating, which it should not be (there should be only 1 timestamp).

@maxmindlin
Copy link
Owner

I think we are about there. Could you resolve the conflicts to allow for the CI to run? Thanks

@maxmindlin
Copy link
Owner

Looks good! Thanks for the contribution

@maxmindlin maxmindlin merged commit 3d31f26 into maxmindlin:master Mar 25, 2023
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.

2 participants