Skip to content

Conversation

groovecoder
Copy link
Member

Add detailed Rust doc comments to RelayClient, RelayAddress, and all public methods to clarify usage, parameters, authentication, and known server limitations for the Relay API client.

Pull Request checklist

  • This PR follows the breaking change policy:
    • This PR has no breaking API changes
  • Quality: This PR builds and tests run cleanly.
  • Tests: This PR only change code comments.
  • Changelog: This PR only changes code comments.
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

Add detailed Rust doc comments to `RelayClient`, `RelayAddress`,
and all public methods to clarify usage, parameters, authentication,
and known server limitations for the Relay API client.
@mattreaganmozilla
Copy link

@groovecoder Thank you for adding these docs 🚀

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

gotta love better docs, thanks!

/// usage stats, and identifying information.
///
/// See:
/// https://dev.fxprivaterelay.nonprod.cloudops.mozgcp.net/api/v1/docs/redoc/#tag/emails/operation/relayaddresses_retrieve
Copy link
Member

Choose a reason for hiding this comment

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

should this link to a .md file or similar in some git repo? This url smells fragile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I've opened mozilla/fx-private-relay#5903 to publish the API docs to the Relay repo's GitHub Pages site. When we get that url working I'll update this.

/// managing authorization to call protected endpoints.
///
/// # Authorization
/// - Clients should user the `FirefoxAccount.getAccessToken()` function
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea if/how any of this works in terms of rustdocs etc 😅 - but maybe fxa_client::FirefoxAccount::getAccessToken()? I doubt either of these will render a workable link though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's the path to use, but also with brackets: [fxa_client::FirefoxAccount::getAccessToken()]. That should render a correctlink in rustdoc. It won't work for the generated Kotlin/Swift/JS docs, but it's something we could eventually get working.

Also: the parens are optional and backticks are allowed, but ignored.
https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I thought about trying to figure it out, but then even if I got the rust docs to link together, I doubt it would carry thru to the Swift or Kotlin docs too, which is the main goal here.

Copy link
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

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

Looks good to me, other than the link issue. I don't need to re-review that though.

/// managing authorization to call protected endpoints.
///
/// # Authorization
/// - Clients should user the `FirefoxAccount.getAccessToken()` function
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's the path to use, but also with brackets: [fxa_client::FirefoxAccount::getAccessToken()]. That should render a correctlink in rustdoc. It won't work for the generated Kotlin/Swift/JS docs, but it's something we could eventually get working.

Also: the parens are optional and backticks are allowed, but ignored.
https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html

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.

4 participants