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

Clean up feature/module documentation with docs.rs features #518

Merged
merged 6 commits into from
Jan 30, 2025

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Jan 30, 2025

run with RUSTDOCFLAGS="--cfg docsrs" cargo doc --all-features --open

  • match send module feature gating to receive module gating. I like this because it makes what's available explicit, and hides stuff you don't want in the default configuration, but I could understand removing if it seems like excessive complexity in cfging (API changing)
  • Put explicit feature labels on optional modules with cfg_attr (this needs the RUSTDOCFLAGS)
  • Remove public feature-specific content-type consts. They're encapsulated in the state machine now. (API changing)
  • remove obsolete base64 re-export since our state machine covers it. (API changing)
  • Add module docs to public modules without fuss
  • Get rid of feature switching in doc strings.

Discovered these flaws with docs in while addressing #504.

`receive` already does this. Make send reflect receive visibility.
@DanGould DanGould requested a review from spacebear21 January 30, 2025 05:26
Indicate modules enabled by feature flags.
These are encapsulated in the Request impl and pollute the docs.
base64 handling is done inside the state machine now.
@coveralls
Copy link
Collaborator

coveralls commented Jan 30, 2025

Pull Request Test Coverage Report for Build 13059298349

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 78.544%

Totals Coverage Status
Change from base Build 13058930455: 0.0%
Covered Lines: 3646
Relevant Lines: 4642

💛 - Coveralls

Describe with a broad brush to improve them from nothing.
@DanGould DanGould added the api label Jan 30, 2025
//!
//! For most use cases, we recommended enabling the `v2` feature, as it is
//! backwards compatible and provides the most convenient experience for users and implementors.
#![cfg_attr(feature = "v2", doc = "To use version 2, refer to [`v2`] module documentation.")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this supposed to only display if the v2 feature is enabled? I tried with RUSTDOCFLAGS="--cfg docsrs" cargo doc --no-default-features --features=v1 --open but this section still shows up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is wild. I'm getting this --no-default-features disrespect as well, even after pruning RUSTDOCFLAGS and
this from Config.toml.

[package.metadata.docs.rs]
all-features = true
rustdoc-args = ["--cfg", "docsrs"]

A simpler solution is to ignore module-specific docs at this top level and instead write it in the module's first line, since we'll post that via all-features to docs.rs where the vast majority will go for documentation.

Comment on lines 9 to 10
//! If you specifically need to use
//! version 1, refer to the [`v1`] module documentation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Relatedly, should this be feature-gated behind the v1 feature being explicitly enabled?

@DanGould
Copy link
Contributor Author

DanGould commented Jan 30, 2025

I got rid of the feature switched documentation. The primary point of features is to minimize compiled code. Limited API surface is a secondary concern, and our documentation may be explicit about available features even when they're available under specific configuration.

@spacebear21
Copy link
Collaborator

An unrelated Url change snuck into the last commit, otherwise looks good to me.

@DanGould
Copy link
Contributor Author

I was wondering where the hell that file went in my stash ... fixed

Excessive feature gates lead to needless confusion.
Copy link
Collaborator

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

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

ACK 3b9eee3

@spacebear21 spacebear21 merged commit 5e402c6 into payjoin:master Jan 30, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants