Skip to content
This repository was archived by the owner on Sep 4, 2024. It is now read-only.

Conversation

@tcharding
Copy link
Collaborator

Audit the whole codebase and improved rustdocs by doing:

  • Use third person tense
  • Use full stops
  • Use backticks on links
  • Use standard rustdoc section headings

src/lib.rs Outdated

/// Shorthand method to convert an argument into a [Box<serde_json::value::RawValue>].
/// Since serializers rarely fail, it's probably easier to use [arg] instead.
/// Shorthand method to convert an argument into a [`Box<serde_json::value::RawValue>`].
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work? I thought links can only point to identifiers. Taking user to Box is not very useful here, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Woops, changed to "... a boxed [serde_json::value::RawValue]" (but with backticks)

/// Since serializers rarely fail, it's probably easier to use [arg] instead.
/// Shorthand method to convert an argument into a [`Box<serde_json::value::RawValue>`].
///
/// Since serializers rarely fail, it's probably easier to use [`arg`] instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just arg, no? It's not a identifier so it can't be a link.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one is correct, arg is a function as well. This threw me too, perhaps we should change the paramater identifier to not be arg?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooooooooohhhh. :D

//! This module implements a minimal and non standard conforming HTTP 1.0
//! round-tripper that works with the bitcoind RPC server. This can be used
//! if minimal dependencies are a goal and synchronous communication is ok.
//!
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did this so every file has the same comment layout. This is copied from rust-bitcoin where I also did it to make everything uniform - pretty anal I know. I did these docs fixes in preparation for bring this repo into the rust-bitcoin org.


impl SimpleHttpTransport {
/// Construct a new `SimpleHttpTransport` with default parameters
/// Constructs a new [`SimpleHttpTransport`] with default parameters.
Copy link
Contributor

Choose a reason for hiding this comment

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

Imperative mood is idiomatic, AFAIK.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I certainly hope not, I've been changing everything to third person for the last 10 months in rust-bitcoin :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh my...

... I just checked stdlib ... and indeed ...

... I've been living a lie.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:)

Copy link
Contributor

@dpc dpc left a comment

Choose a reason for hiding this comment

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

Please check with env RUSTDOCFLAGS='-D rustdoc::broken_intra_doc_links' cargo doc --no-deps (probably good idea to have it in CI)

src/lib.rs Outdated
}

/// Shorthand method to convert an argument into a [Box<serde_json::value::RawValue>].
/// Shorthand method to convert an argument into a [`Box<serde_json::value::RawValue>`].
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cheers

@tcharding
Copy link
Collaborator Author

Thanks for the review @dpc! Hope my responses weren't too terse, I'm in the last few minutes of my day.

@tcharding
Copy link
Collaborator Author

Force push with no changes (change to commit hash only) to trigger CI.

@tcharding
Copy link
Collaborator Author

tcharding commented Nov 16, 2022

Will need #74 for the clippy CI fails, for the MacOS fails I don't know whats wrong.

@tcharding
Copy link
Collaborator Author

tcharding commented Nov 16, 2022

Please check with env RUSTDOCFLAGS='-D rustdoc::broken_intra_doc_links' cargo doc --no-deps (probably good idea to have it in CI)

Ha! We use this already in this repo but by pure coincidence I stumbled across broken links in rust-secp256k1 this morning and your fix was fresh in my mind - so sweet.

@apoelstra
Copy link
Owner

Needs rebase to resolve conflicts and to fix CI.

Audit the whole codebase and improved rustdocs by doing:

- Use third person tense
- Use full stops
- Use backticks on links
- Use standard rustdoc section headings
Linking to `Box` in the rustdocs is pointless, link to the inner
`serder_json` type instead.
@tcharding
Copy link
Collaborator Author

Cheers, rebased. No other changes.

Copy link
Owner

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK ab44396

@apoelstra apoelstra merged commit 7ea2199 into apoelstra:master Nov 18, 2022
@tcharding tcharding deleted the 11-16-docs branch November 20, 2022 23:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants