ZettaScaleLabs / stabby

A Stable ABI for Rust with compact sum-types
Other
328 stars 13 forks source link

Improve documentation #31

Closed Dragonink closed 8 months ago

Dragonink commented 1 year ago

Fixes #30, although I guess documenting _U128 as 0 whereas it panics in real code is misleading. Maybe we should add a documentation line for this, what do you think?

Also I forced to document API behind features (like stabby::libloading) because I found some items in your examples that did not appear on docs.rs.

I state that I was not able to sign ECA with my GitHub private email, since I wouldn't want to disclose my public email for this particular commit, and that therefore I grant the permission to copy/merge my PR if accepted

p-avital commented 1 year ago

Hi @Dragonink, thanks a lot, that's lovely (I was just trying that 0 trick locally). While I'd agree that documenting it as 0 is a bit misleading, it panics at compile time (which is why docs wasn't passing), so it's not like it's gonna introduce surprise panics at runtime. Of course, adding a /// Evaluating \Saturator::UX` will cause a compile-time error` to the docs of the appropriate fields could be a way to remove the surprise entirely.

Just checking: do you plan on adding anything else to this PR, or are you happy to have it merged as-is? :)

Dragonink commented 1 year ago

I just found another problem while documenting a crate where I added stabby as a dependency with default-features disabled. It has to do with undeclared crates, namely alloc and libloading.

I found a fix for alloc that I will push, but I don't know if I can fix the libloading one without enabling that feature. Do you have an idea?

Dragonink commented 1 year ago

Ok, I think I'll go back to

#[cfg(all(feature = "libloading", any(unix, windows)))]
pub mod libloading;

and just add

[package.metadata.docs.rs]
all-features = true
p-avital commented 1 year ago

Could you provide a reproduction example for the issue you're facing? I'm always curious about these :)

Also, I just checked and cfg(doc) is now stable, so I think the #[feature(doc_auto_cfg)] is unnecessary, as are the anys you added, since cargo doc should build with all features enabled IIRC :)

Dragonink commented 1 year ago

Also, I just checked and cfg(doc) is now stable, so I think the #[feature(doc_auto_cfg)] is unnecessary

cfg(doc) is stable, but doc(cfg) is not. That's what feature(doc_auto_cfg) is used for.

p-avital commented 8 months ago

After much work on docs, I think this PR is no longer necessary. Thanks for making me aware of the issue :)