Skip to content

Conversation

jlizen
Copy link
Member

@jlizen jlizen commented Apr 21, 2025

Still need to test locally - just was getting the PR open before closing up for the day :)

@jlizen
Copy link
Member Author

jlizen commented Apr 21, 2025

Re: testing, i see attributes defined on the items both in their root crate export: https://github.com/tokio-rs/tokio-metrics/blob/main/src/lib.rs#L165 as well as on their direct declaration: https://github.com/tokio-rs/tokio-metrics/blob/main/src/runtime.rs#L8

So wanted to double check whether both are required, which one, etc.

If it's needed everywhere, I can take a pass to reuse the cfg_rt! macro inside the module (and add a similar cfg_metrics_rs_rt!. If only in the root, I can clean up the vestigial annotations inside the module.

@arielb1
Copy link
Contributor

arielb1 commented Apr 22, 2025

Approving if this is tested in docs.rs. If you figure out how to generate docs.rs like docs, could you put these instructions in a CONTRIBUTING.md file?

@jlizen jlizen changed the title fix: rt: in docs.rs, annotate the module as requiring metrics-rs-int…egration along with rt and tokio_unstable fix: rt: in docs.rs, annotate the module as requiring metrics-rs-integration along with rt and tokio_unstable Apr 23, 2025
@jlizen
Copy link
Member Author

jlizen commented Apr 23, 2025

@arielb1 added test instructions to CONTRIBUTING.md, it's pretty easy. I did validate that the annotations on the direct struct definitions are not needed if you annotate the re-exports from lib.rs only.

I'll go ahead and merge, but I'm not sure if we need manual action to stage this to rustdocs? Is our automation hooked into crates.io publish only?

@jlizen
Copy link
Member Author

jlizen commented Apr 23, 2025

(From ariel offline - This will need to wait to go out until next crates.io version, probably not worth it just for this - i'll cut a tracking issue)

@jlizen jlizen merged commit 5c61d50 into tokio-rs:main Apr 23, 2025
5 checks passed
@jlizen jlizen mentioned this pull request Apr 23, 2025
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