-
Notifications
You must be signed in to change notification settings - Fork 361
fix(docs): enable all features in docs.rs build #1000
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
Conversation
Tacking on another small doc bug fix, we were missing an |
Yes I agree, what would be the impact if this is not working? I think since specific features are already not showing up, I think that's OK. |
Basically just missing docs for the non-default features. Since we have a CI step now validating that the doc build itself passes, and it already builds all features. I have high confidence that it will work though, these are standard metadata flags. |
@maxday i dogpiled on some missing feature annotations for docs.rs, didn't see you had already reviewed, sorry! it's in a separate commit |
Not a strong opinion but I feel this is quite error prone with the need of the string twice. What do you think about having a macro for this (like tokio does: https://github.com/tokio-rs/tokio/blob/9563707aaa73a802fa4d3c51c12869a037641070/tokio-util/src/cfg.rs#L1). |
I'm pretty lukewarm on this. It makes sense for tokio because they have a more central entry point, fewer features, and need to add the tag in various inner places due to complex public in private module visibility handling to seal certain types. In contrast we have like 50 features given all the events, so that's a lot of macros. For the ones that we only use like twice (ie the events ones), probably not worth it. For tracing / similar... Maybe? It's a bit awkward since we would need to redefine the macro in multiple crates if we don't want to export it publicly. I don't have time to implement the macro right this moment but I'd be fine to remove the commit adding all the missing annotations from this PR, if we would rather wait on that to add. |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@bnusunny looks like a doc ci run is failing, not sure why that didn't break previously, I'll fix tomorrow. Please don't merge yet. |
@bnusunny should be good to go now (though I can't directly validate as my fork's CI didn't fail... however |
📬 Issue #, if available:
n/a
✍️ Description of changes:
I noticed that our docs.rs docs were only showing default features. Adding the metadata tag to all crates. (Unfortunately these settings are not supported in workspace
Cargo.toml
.As best as I can tell, we have no directly conflicting features, so I enabled all features to help future-proof. This is most relevant to
lambda_events
.I didn't directly test the docs.rs build since it's a bit annoying, you have to spin up their server locally. I can if the reviewer prefers though, not a big deal.
🔏 By submitting this pull request
cargo +nightly fmt
.cargo clippy --fix
.