Skip to content

Fix trivial doc warnings and make CI use old rustc #955

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

Merged
merged 4 commits into from
Jun 18, 2021

Conversation

TheBlueMatt
Copy link
Collaborator

rustc stable regressed in creating docs, see rust-lang/rust#84738. This causes our docs to fail to build. This isn't our problem, really, but we shouldn't be failing CI while rustc gets its act together.

These constants are generally useful, and are linked from
documentation, so should be exposed in any case.
@codecov
Copy link

codecov bot commented Jun 17, 2021

Codecov Report

Merging #955 (f875579) into main (2940099) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #955      +/-   ##
==========================================
- Coverage   90.59%   90.57%   -0.02%     
==========================================
  Files          60       60              
  Lines       30423    30423              
==========================================
- Hits        27561    27555       -6     
- Misses       2862     2868       +6     
Impacted Files Coverage Δ
lightning-invoice/src/lib.rs 88.49% <ø> (ø)
lightning/src/util/message_signing.rs 93.10% <ø> (ø)
lightning/src/ln/functional_tests.rs 97.16% <0.00%> (-0.09%) ⬇️
lightning/src/ln/channelmanager.rs 83.85% <0.00%> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2940099...f875579. Read the comment docs.

@@ -12,13 +12,13 @@
//! Note this is not part of the specs, but follows lnd's signing and verifying protocol, which can is defined as follows:
//!
//! signature = zbase32(SigRec(sha256d(("Lightning Signed Message:" + msg)))
//! zbase32 from https://philzimmermann.com/docs/human-oriented-base-32-encoding.txt
//! zbase32 from `<https://philzimmermann.com/docs/human-oriented-base-32-encoding.txt>`
Copy link

Choose a reason for hiding this comment

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

This doesn't create a hyperlink, it shows it as code: <https://philzimmermann.com/docs/human-oriented-base-32-encoding.txt>

Suggested change
//! zbase32 from `<https://philzimmermann.com/docs/human-oriented-base-32-encoding.txt>`
//! zbase32 from <https://philzimmermann.com/docs/human-oriented-base-32-encoding.txt>

Copy link
Collaborator Author

@TheBlueMatt TheBlueMatt Jun 17, 2021

Choose a reason for hiding this comment

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

Ah! Thanks. Is there a feature request to change the warning message to drop the "`"s on rustdoc, or should I open one? Was definitely confusing for me, I wonder if there's a better way to communicate that the "`" is intended as a quote, not a part of the suggestion.

Copy link

Choose a reason for hiding this comment

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

There's no issue, you should open a new one.

That seems be hard to do, this is part of diagnostic machinery. How would you decide what suggestions get a backtick and which don't?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I went and read some of the rustc source after commenting and realized its probably not trivial. I'll leave it alone for now, I was hoping it was pretty trivial and I could open a feature request someone could knock out quick, but I don't want to go demand something thats a papercut and takes significant effort to fix.

Latest rustc warns that "this URL is not a hyperlink" and notes that
"bare URLs are not automatically turned into clickable links". This
resolves those warnings.

Thanks to Joshua Nelson for pointing out the correct syntax for
this.

/// Default minimum final CLTV expiry as defined by [BOLT 11].
///
/// [BOLT 11]: https://github.com/lightningnetwork/lightning-rfc/blob/master/11-payment-encoding.md
const DEFAULT_MIN_FINAL_CLTV_EXPIRY: u64 = 18;
pub const DEFAULT_MIN_FINAL_CLTV_EXPIRY: u64 = 18;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change this constant since RL isn't okay with less than 22 (iiuc)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the default value is always 18, we require something higher but that means we have to explicitly specify it in the invoice (which we do).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a comment making this explicit as users are likely to be confused here.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

ACK f875579

@TheBlueMatt TheBlueMatt merged commit 244ad83 into lightningdevkit:main Jun 18, 2021
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