Skip to content

Docs improvements - #2100

Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
douglaz:docs_fixes
Mar 21, 2023
Merged

Docs improvements#2100
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
douglaz:docs_fixes

Conversation

@douglaz

@douglazdouglaz commented Mar 12, 2023

Copy link
Copy Markdown
Contributor

I was looking at the documentation and found some missing references. Doing that I found minor "bugs" (wrong references). See below for my double check comments.

Please note this change isn't exhaustive, there are still many places where similar changes can be made.

douglaz
douglaz commented Mar 12, 2023
View reviewed changes
Comment threadfuzz/src/full_stack.rs
douglaz
douglaz commented Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs Outdated
douglaz
douglaz commented Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs Outdated
douglaz
douglaz commented Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
douglaz
douglaz commented Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs Outdated
@codecov-commenter

codecov-commenter commented Mar 12, 2023•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Patch and project coverage have no change.

Comparison is base (2d213a4) 91.15% compared to head (060a399) 91.15%.

❗ Current head 060a399 differs from pull request most recent head 963d6c4. Consider uploading reports for the commit 963d6c4 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@ Coverage Diff @@## main #2100 +/- ##
=======================================
Coverage 91.15% 91.15% =======================================
Files 101 101 Lines 48866 48866 Branches 48866 48866 =======================================
Hits 44544 44544 Misses 4322 4322
Impacted FilesCoverage Δ
lightning-invoice/src/utils.rs96.90% <ø> (ø)
lightning/src/ln/chan_utils.rs93.75% <ø> (ø)
lightning/src/ln/channelmanager.rs88.85% <ø> (ø)
lightning/src/ln/mod.rs95.00% <ø> (ø)
lightning/src/ln/peer_handler.rs62.62% <ø> (ø)
lightning/src/routing/gossip.rs89.63% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

tnull
tnull reviewed Mar 13, 2023
View reviewed changes

@tnulltnull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for having a go at this!

I think generally we want to tick or link all references to objects in doc comments. However, for the sake of readability it should generally be enough to link the first occurrence in a paragraph and only tick the rest of the references (there are of course exceptions to this, so def. feel free to link when you think it's needed).

Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/peer_handler.rs Outdated
Comment threadlightning/src/ln/peer_handler.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
douglaz
douglaz commented Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
tnull
tnull reviewed Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
TheBlueMatt
TheBlueMatt reviewed Mar 14, 2023
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
@douglaz
douglazforce-pushed the docs_fixes branch 2 times, most recently from e84874e to 060a399CompareMarch 14, 2023 19:54
@douglazdouglaz mentioned this pull request Mar 14, 2023
Merged
@douglaz

douglaz commented Mar 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Addressed all review comments and moved channel.rs changes to #2104

tnull
tnull reviewed Mar 15, 2023
View reviewed changes

@tnulltnull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.

@douglaz

douglaz commented Mar 15, 2023

Copy link
Copy Markdown
ContributorAuthor

LGTM.

One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.

I think a PR focused on that would be better. I've created #2105
Could you label it good first issue?

@tnull

tnull commented Mar 15, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

I think a PR focused on that would be better. I've created #2105 Could you label it good first issue?

SGTM, even though it might introduce some additional git noise.

TheBlueMatt
TheBlueMatt reviewed Mar 18, 2023
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, your cleanups turned up a bunch of places where the docs are just downright ancient and need updating :)

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/mod.rs Outdated
@douglaz

douglaz commented Mar 20, 2023

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt Please double check the changes

TheBlueMatt
TheBlueMatt reviewed Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs Outdated
@douglaz

douglaz commented Mar 21, 2023

Copy link
Copy Markdown
ContributorAuthor

I think it's okay now 🤞

tnull
tnull reviewed Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/peer_handler.rs Outdated
Comment threadlightning/src/ln/mod.rs Outdated
@douglaz

douglaz commented Mar 21, 2023

Copy link
Copy Markdown
ContributorAuthor

@tnull done

@TheBlueMatt
TheBlueMatt merged commit ea15f0f into lightningdevkit:mainMar 21, 2023
@douglaz
douglaz deleted the docs_fixes branch March 21, 2023 18:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tnulltnulltnull approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@douglaz@codecov-commenter@tnull@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Docs improvements by douglaz · Pull Request #2100 · lightningdevkit/rust-lightning · GitHub
Skip to content
Docs improvements - #2100
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
douglaz:docs_fixes
Mar 21, 2023
Merged
Docs improvements#2100
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
douglaz:docs_fixes
Conversation
@douglaz
@douglazdouglaz
commented
Mar 12, 2023

Copy link


Copy Markdown

Contributor
I was looking at the documentation and found some missing references. Doing that I found minor "bugs" (wrong references). See below for my double check comments.
Please note this change isn't exhaustive, there are still many places where similar changes can be made.
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadfuzz/src/full_stack.rs
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
Outdated

douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@codecov-commenter
codecov-commenter
commented
Mar 12, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch and project coverage have no change.
Comparison is base (2d213a4) 91.15% compared to head (060a399) 91.15%.
❗ Current head 060a399 differs from pull request most recent head 963d6c4. Consider uploading reports for the commit 963d6c4 to get more accurate results
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
Additional details and impacted files
@@ Coverage Diff @@## main #2100 +/- ##
=======================================
Coverage 91.15% 91.15% =======================================
Files 101 101 Lines 48866 48866 Branches 48866 48866 =======================================
Hits 44544 44544 Misses 4322 4322 
Impacted FilesCoverage Δ
lightning-invoice/src/utils.rs96.90% <ø> (ø)
lightning/src/ln/chan_utils.rs93.75% <ø> (ø)
lightning/src/ln/channelmanager.rs88.85% <ø> (ø)
lightning/src/ln/mod.rs95.00% <ø> (ø)
lightning/src/ln/peer_handler.rs62.62% <ø> (ø)
lightning/src/routing/gossip.rs89.63% <ø> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report in Codecov by Sentry.

📢 Do you have feedback about the report comment? Let us know in this issue.
tnull
tnull
reviewed
Mar 13, 2023
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for having a go at this!
I think generally we want to tick or link all references to objects in doc comments. However, for the sake of readability it should generally be enough to link the first occurrence in a paragraph and only tick the rest of the references (there are of course exceptions to this, so def. feel free to link when you think it's needed).
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/chan_utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

douglaz
douglaz
commented
Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
tnull
tnull
reviewed
Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 14, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Thanks!
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@douglaz
douglazforce-pushed
the
docs_fixes
branch
2 times, most recently
from
e84874e to
060a399CompareMarch 14, 2023 19:54
@douglazdouglaz
mentioned this pull request
Mar 14, 2023
 Merged

@douglaz
douglaz
commented
Mar 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed all review comments and moved channel.rs changes to #2104
tnull
tnull
reviewed
Mar 15, 2023
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM.
One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.
@douglaz
douglaz
commented
Mar 15, 2023

Copy link


Copy Markdown

ContributorAuthor
LGTM.
One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.
I think a PR focused on that would be better. I've created #2105

Could you label it good first issue?
@tnull
tnull
commented
Mar 15, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I think a PR focused on that would be better. I've created #2105 Could you label it good first issue?
SGTM, even though it might introduce some additional git noise.
TheBlueMatt
TheBlueMatt
reviewed
Mar 18, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Thanks, your cleanups turned up a bunch of places where the docs are just downright ancient and need updating :)
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/mod.rs
Outdated

@douglaz
douglaz
commented
Mar 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt Please double check the changes
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@douglaz
douglaz
commented
Mar 21, 2023

Copy link


Copy Markdown

ContributorAuthor
I think it's okay now 🤞
tnull
tnull
reviewed
Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/mod.rs
Outdated

@douglaz
douglaz
commented
Mar 21, 2023

Copy link


Copy Markdown

ContributorAuthor
@tnull done
@TheBlueMatt
TheBlueMatt
merged commit ea15f0f
into
lightningdevkit:mainMar 21, 2023
@douglaz
douglaz
deleted the

docs_fixes

branch
March 21, 2023 18:55
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@douglaz@codecov-commenter@tnull@TheBlueMatt

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Docs improvements by douglaz · Pull Request #2100 · lightningdevkit/rust-lightning · GitHub
Skip to content
Docs improvements - #2100
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
douglaz:docs_fixes
Mar 21, 2023
Merged
Docs improvements#2100
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
douglaz:docs_fixes
Conversation
@douglaz
@douglazdouglaz
commented
Mar 12, 2023

Copy link


Copy Markdown

Contributor
I was looking at the documentation and found some missing references. Doing that I found minor "bugs" (wrong references). See below for my double check comments.
Please note this change isn't exhaustive, there are still many places where similar changes can be made.
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadfuzz/src/full_stack.rs
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
Outdated

douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@codecov-commenter
codecov-commenter
commented
Mar 12, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch and project coverage have no change.
Comparison is base (2d213a4) 91.15% compared to head (060a399) 91.15%.
❗ Current head 060a399 differs from pull request most recent head 963d6c4. Consider uploading reports for the commit 963d6c4 to get more accurate results
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
Additional details and impacted files
@@ Coverage Diff @@## main #2100 +/- ##
=======================================
Coverage 91.15% 91.15% =======================================
Files 101 101 Lines 48866 48866 Branches 48866 48866 =======================================
Hits 44544 44544 Misses 4322 4322 
Impacted FilesCoverage Δ
lightning-invoice/src/utils.rs96.90% <ø> (ø)
lightning/src/ln/chan_utils.rs93.75% <ø> (ø)
lightning/src/ln/channelmanager.rs88.85% <ø> (ø)
lightning/src/ln/mod.rs95.00% <ø> (ø)
lightning/src/ln/peer_handler.rs62.62% <ø> (ø)
lightning/src/routing/gossip.rs89.63% <ø> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report in Codecov by Sentry.

📢 Do you have feedback about the report comment? Let us know in this issue.
tnull
tnull
reviewed
Mar 13, 2023
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for having a go at this!
I think generally we want to tick or link all references to objects in doc comments. However, for the sake of readability it should generally be enough to link the first occurrence in a paragraph and only tick the rest of the references (there are of course exceptions to this, so def. feel free to link when you think it's needed).
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/chan_utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

douglaz
douglaz
commented
Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
tnull
tnull
reviewed
Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 14, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Thanks!
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@douglaz
douglazforce-pushed
the
docs_fixes
branch
2 times, most recently
from
e84874e to
060a399CompareMarch 14, 2023 19:54
@douglazdouglaz
mentioned this pull request
Mar 14, 2023
 Merged

@douglaz
douglaz
commented
Mar 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed all review comments and moved channel.rs changes to #2104
tnull
tnull
reviewed
Mar 15, 2023
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM.
One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.
@douglaz
douglaz
commented
Mar 15, 2023

Copy link


Copy Markdown

ContributorAuthor
LGTM.
One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.
I think a PR focused on that would be better. I've created #2105

Could you label it good first issue?
@tnull
tnull
commented
Mar 15, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I think a PR focused on that would be better. I've created #2105 Could you label it good first issue?
SGTM, even though it might introduce some additional git noise.
TheBlueMatt
TheBlueMatt
reviewed
Mar 18, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Thanks, your cleanups turned up a bunch of places where the docs are just downright ancient and need updating :)
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/mod.rs
Outdated

@douglaz
douglaz
commented
Mar 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt Please double check the changes
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@douglaz
douglaz
commented
Mar 21, 2023

Copy link


Copy Markdown

ContributorAuthor
I think it's okay now 🤞
tnull
tnull
reviewed
Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/mod.rs
Outdated

@douglaz
douglaz
commented
Mar 21, 2023

Copy link


Copy Markdown

ContributorAuthor
@tnull done
@TheBlueMatt
TheBlueMatt
merged commit ea15f0f
into
lightningdevkit:mainMar 21, 2023
@douglaz
douglaz
deleted the

docs_fixes

branch
March 21, 2023 18:55
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@douglaz@codecov-commenter@tnull@TheBlueMatt

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Docs improvements by douglaz · Pull Request #2100 · lightningdevkit/rust-lightning · GitHub
Skip to content
Docs improvements - #2100
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
douglaz:docs_fixes
Mar 21, 2023
Merged
Docs improvements#2100
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
douglaz:docs_fixes
Conversation
@douglaz
@douglazdouglaz
commented
Mar 12, 2023

Copy link


Copy Markdown

Contributor
I was looking at the documentation and found some missing references. Doing that I found minor "bugs" (wrong references). See below for my double check comments.
Please note this change isn't exhaustive, there are still many places where similar changes can be made.
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadfuzz/src/full_stack.rs
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
Outdated

douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@codecov-commenter
codecov-commenter
commented
Mar 12, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch and project coverage have no change.
Comparison is base (2d213a4) 91.15% compared to head (060a399) 91.15%.
❗ Current head 060a399 differs from pull request most recent head 963d6c4. Consider uploading reports for the commit 963d6c4 to get more accurate results
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
Additional details and impacted files
@@ Coverage Diff @@## main #2100 +/- ##
=======================================
Coverage 91.15% 91.15% =======================================
Files 101 101 Lines 48866 48866 Branches 48866 48866 =======================================
Hits 44544 44544 Misses 4322 4322 
Impacted FilesCoverage Δ
lightning-invoice/src/utils.rs96.90% <ø> (ø)
lightning/src/ln/chan_utils.rs93.75% <ø> (ø)
lightning/src/ln/channelmanager.rs88.85% <ø> (ø)
lightning/src/ln/mod.rs95.00% <ø> (ø)
lightning/src/ln/peer_handler.rs62.62% <ø> (ø)
lightning/src/routing/gossip.rs89.63% <ø> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report in Codecov by Sentry.

📢 Do you have feedback about the report comment? Let us know in this issue.
tnull
tnull
reviewed
Mar 13, 2023
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for having a go at this!
I think generally we want to tick or link all references to objects in doc comments. However, for the sake of readability it should generally be enough to link the first occurrence in a paragraph and only tick the rest of the references (there are of course exceptions to this, so def. feel free to link when you think it's needed).
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/chan_utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

douglaz
douglaz
commented
Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
tnull
tnull
reviewed
Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 14, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Thanks!
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@douglaz
douglazforce-pushed
the
docs_fixes
branch
2 times, most recently
from
e84874e to
060a399CompareMarch 14, 2023 19:54
@douglazdouglaz
mentioned this pull request
Mar 14, 2023
 Merged

@douglaz
douglaz
commented
Mar 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed all review comments and moved channel.rs changes to #2104
tnull
tnull
reviewed
Mar 15, 2023
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM.
One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.
@douglaz
douglaz
commented
Mar 15, 2023

Copy link


Copy Markdown

ContributorAuthor
LGTM.
One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.
I think a PR focused on that would be better. I've created #2105

Could you label it good first issue?
@tnull
tnull
commented
Mar 15, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I think a PR focused on that would be better. I've created #2105 Could you label it good first issue?
SGTM, even though it might introduce some additional git noise.
TheBlueMatt
TheBlueMatt
reviewed
Mar 18, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Thanks, your cleanups turned up a bunch of places where the docs are just downright ancient and need updating :)
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/mod.rs
Outdated

@douglaz
douglaz
commented
Mar 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt Please double check the changes
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@douglaz
douglaz
commented
Mar 21, 2023

Copy link


Copy Markdown

ContributorAuthor
I think it's okay now 🤞
tnull
tnull
reviewed
Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/mod.rs
Outdated

@douglaz
douglaz
commented
Mar 21, 2023

Copy link


Copy Markdown

ContributorAuthor
@tnull done
@TheBlueMatt
TheBlueMatt
merged commit ea15f0f
into
lightningdevkit:mainMar 21, 2023
@douglaz
douglaz
deleted the

docs_fixes

branch
March 21, 2023 18:55
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@douglaz@codecov-commenter@tnull@TheBlueMatt

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Docs improvements by douglaz · Pull Request #2100 · lightningdevkit/rust-lightning · GitHub
Skip to content
Docs improvements - #2100
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
douglaz:docs_fixes
Mar 21, 2023
Merged
Docs improvements#2100
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
douglaz:docs_fixes
Conversation
@douglaz
@douglazdouglaz
commented
Mar 12, 2023

Copy link


Copy Markdown

Contributor
I was looking at the documentation and found some missing references. Doing that I found minor "bugs" (wrong references). See below for my double check comments.
Please note this change isn't exhaustive, there are still many places where similar changes can be made.
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadfuzz/src/full_stack.rs
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
Outdated

douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@codecov-commenter
codecov-commenter
commented
Mar 12, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch and project coverage have no change.
Comparison is base (2d213a4) 91.15% compared to head (060a399) 91.15%.
❗ Current head 060a399 differs from pull request most recent head 963d6c4. Consider uploading reports for the commit 963d6c4 to get more accurate results
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
Additional details and impacted files
@@ Coverage Diff @@## main #2100 +/- ##
=======================================
Coverage 91.15% 91.15% =======================================
Files 101 101 Lines 48866 48866 Branches 48866 48866 =======================================
Hits 44544 44544 Misses 4322 4322 
Impacted FilesCoverage Δ
lightning-invoice/src/utils.rs96.90% <ø> (ø)
lightning/src/ln/chan_utils.rs93.75% <ø> (ø)
lightning/src/ln/channelmanager.rs88.85% <ø> (ø)
lightning/src/ln/mod.rs95.00% <ø> (ø)
lightning/src/ln/peer_handler.rs62.62% <ø> (ø)
lightning/src/routing/gossip.rs89.63% <ø> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report in Codecov by Sentry.

📢 Do you have feedback about the report comment? Let us know in this issue.
tnull
tnull
reviewed
Mar 13, 2023
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for having a go at this!
I think generally we want to tick or link all references to objects in doc comments. However, for the sake of readability it should generally be enough to link the first occurrence in a paragraph and only tick the rest of the references (there are of course exceptions to this, so def. feel free to link when you think it's needed).
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/chan_utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

douglaz
douglaz
commented
Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
tnull
tnull
reviewed
Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 14, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Thanks!
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@douglaz
douglazforce-pushed
the
docs_fixes
branch
2 times, most recently
from
e84874e to
060a399CompareMarch 14, 2023 19:54
@douglazdouglaz
mentioned this pull request
Mar 14, 2023
 Merged

@douglaz
douglaz
commented
Mar 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed all review comments and moved channel.rs changes to #2104
tnull
tnull
reviewed
Mar 15, 2023
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM.
One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.
@douglaz
douglaz
commented
Mar 15, 2023

Copy link


Copy Markdown

ContributorAuthor
LGTM.
One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.
I think a PR focused on that would be better. I've created #2105

Could you label it good first issue?
@tnull
tnull
commented
Mar 15, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I think a PR focused on that would be better. I've created #2105 Could you label it good first issue?
SGTM, even though it might introduce some additional git noise.
TheBlueMatt
TheBlueMatt
reviewed
Mar 18, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Thanks, your cleanups turned up a bunch of places where the docs are just downright ancient and need updating :)
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/mod.rs
Outdated

@douglaz
douglaz
commented
Mar 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt Please double check the changes
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@douglaz
douglaz
commented
Mar 21, 2023

Copy link


Copy Markdown

ContributorAuthor
I think it's okay now 🤞
tnull
tnull
reviewed
Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/mod.rs
Outdated

@douglaz
douglaz
commented
Mar 21, 2023

Copy link


Copy Markdown

ContributorAuthor
@tnull done
@TheBlueMatt
TheBlueMatt
merged commit ea15f0f
into
lightningdevkit:mainMar 21, 2023
@douglaz
douglaz
deleted the

docs_fixes

branch
March 21, 2023 18:55
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@douglaz@codecov-commenter@tnull@TheBlueMatt

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Docs improvements by douglaz · Pull Request #2100 · lightningdevkit/rust-lightning · GitHub
Skip to content
Docs improvements - #2100
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
douglaz:docs_fixes
Mar 21, 2023
Merged
Docs improvements#2100
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
douglaz:docs_fixes
Conversation
@douglaz
@douglazdouglaz
commented
Mar 12, 2023

Copy link


Copy Markdown

Contributor
I was looking at the documentation and found some missing references. Doing that I found minor "bugs" (wrong references). See below for my double check comments.
Please note this change isn't exhaustive, there are still many places where similar changes can be made.
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadfuzz/src/full_stack.rs
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
Outdated

douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@codecov-commenter
codecov-commenter
commented
Mar 12, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch and project coverage have no change.
Comparison is base (2d213a4) 91.15% compared to head (060a399) 91.15%.
❗ Current head 060a399 differs from pull request most recent head 963d6c4. Consider uploading reports for the commit 963d6c4 to get more accurate results
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
Additional details and impacted files
@@ Coverage Diff @@## main #2100 +/- ##
=======================================
Coverage 91.15% 91.15% =======================================
Files 101 101 Lines 48866 48866 Branches 48866 48866 =======================================
Hits 44544 44544 Misses 4322 4322 
Impacted FilesCoverage Δ
lightning-invoice/src/utils.rs96.90% <ø> (ø)
lightning/src/ln/chan_utils.rs93.75% <ø> (ø)
lightning/src/ln/channelmanager.rs88.85% <ø> (ø)
lightning/src/ln/mod.rs95.00% <ø> (ø)
lightning/src/ln/peer_handler.rs62.62% <ø> (ø)
lightning/src/routing/gossip.rs89.63% <ø> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report in Codecov by Sentry.

📢 Do you have feedback about the report comment? Let us know in this issue.
tnull
tnull
reviewed
Mar 13, 2023
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for having a go at this!
I think generally we want to tick or link all references to objects in doc comments. However, for the sake of readability it should generally be enough to link the first occurrence in a paragraph and only tick the rest of the references (there are of course exceptions to this, so def. feel free to link when you think it's needed).
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/chan_utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

douglaz
douglaz
commented
Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
tnull
tnull
reviewed
Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 14, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Thanks!
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@douglaz
douglazforce-pushed
the
docs_fixes
branch
2 times, most recently
from
e84874e to
060a399CompareMarch 14, 2023 19:54
@douglazdouglaz
mentioned this pull request
Mar 14, 2023
 Merged

@douglaz
douglaz
commented
Mar 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed all review comments and moved channel.rs changes to #2104
tnull
tnull
reviewed
Mar 15, 2023
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM.
One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.
@douglaz
douglaz
commented
Mar 15, 2023

Copy link


Copy Markdown

ContributorAuthor
LGTM.
One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.
I think a PR focused on that would be better. I've created #2105

Could you label it good first issue?
@tnull
tnull
commented
Mar 15, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I think a PR focused on that would be better. I've created #2105 Could you label it good first issue?
SGTM, even though it might introduce some additional git noise.
TheBlueMatt
TheBlueMatt
reviewed
Mar 18, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Thanks, your cleanups turned up a bunch of places where the docs are just downright ancient and need updating :)
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/mod.rs
Outdated

@douglaz
douglaz
commented
Mar 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt Please double check the changes
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@douglaz
douglaz
commented
Mar 21, 2023

Copy link


Copy Markdown

ContributorAuthor
I think it's okay now 🤞
tnull
tnull
reviewed
Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/mod.rs
Outdated

@douglaz
douglaz
commented
Mar 21, 2023

Copy link


Copy Markdown

ContributorAuthor
@tnull done
@TheBlueMatt
TheBlueMatt
merged commit ea15f0f
into
lightningdevkit:mainMar 21, 2023
@douglaz
douglaz
deleted the

docs_fixes

branch
March 21, 2023 18:55
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@douglaz@codecov-commenter@tnull@TheBlueMatt

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
})();
Docs improvements by douglaz · Pull Request #2100 · lightningdevkit/rust-lightning · GitHub
Skip to content
Docs improvements - #2100
Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
douglaz:docs_fixes
Mar 21, 2023
Merged
Docs improvements#2100
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom 
douglaz:docs_fixes
Conversation
@douglaz
@douglazdouglaz
commented
Mar 12, 2023

Copy link


Copy Markdown

Contributor
I was looking at the documentation and found some missing references. Doing that I found minor "bugs" (wrong references). See below for my double check comments.
Please note this change isn't exhaustive, there are still many places where similar changes can be made.
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadfuzz/src/full_stack.rs
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
Outdated

douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
douglaz
douglaz
commented
Mar 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@codecov-commenter
codecov-commenter
commented
Mar 12, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch and project coverage have no change.
Comparison is base (2d213a4) 91.15% compared to head (060a399) 91.15%.
❗ Current head 060a399 differs from pull request most recent head 963d6c4. Consider uploading reports for the commit 963d6c4 to get more accurate results
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
Additional details and impacted files
@@ Coverage Diff @@## main #2100 +/- ##
=======================================
Coverage 91.15% 91.15% =======================================
Files 101 101 Lines 48866 48866 Branches 48866 48866 =======================================
Hits 44544 44544 Misses 4322 4322 
Impacted FilesCoverage Δ
lightning-invoice/src/utils.rs96.90% <ø> (ø)
lightning/src/ln/chan_utils.rs93.75% <ø> (ø)
lightning/src/ln/channelmanager.rs88.85% <ø> (ø)
lightning/src/ln/mod.rs95.00% <ø> (ø)
lightning/src/ln/peer_handler.rs62.62% <ø> (ø)
lightning/src/routing/gossip.rs89.63% <ø> (ø)
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report in Codecov by Sentry.

📢 Do you have feedback about the report comment? Let us know in this issue.
tnull
tnull
reviewed
Mar 13, 2023
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for having a go at this!
I think generally we want to tick or link all references to objects in doc comments. However, for the sake of readability it should generally be enough to link the first occurrence in a paragraph and only tick the rest of the references (there are of course exceptions to this, so def. feel free to link when you think it's needed).
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/chan_utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

douglaz
douglaz
commented
Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/chan_utils.rs
tnull
tnull
reviewed
Mar 13, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

TheBlueMatt
TheBlueMatt
reviewed
Mar 14, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Thanks!
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@douglaz
douglazforce-pushed
the
docs_fixes
branch
2 times, most recently
from
e84874e to
060a399CompareMarch 14, 2023 19:54
@douglazdouglaz
mentioned this pull request
Mar 14, 2023
 Merged

@douglaz
douglaz
commented
Mar 14, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed all review comments and moved channel.rs changes to #2104
tnull
tnull
reviewed
Mar 15, 2023
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM.
One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.
@douglaz
douglaz
commented
Mar 15, 2023

Copy link


Copy Markdown

ContributorAuthor
LGTM.
One optional thing you could tackle while you're here is to ensure that any (C-not exported) lines come after a blank line, as these comments are not meant for humans and they'll end up in the module-level docs otherwise. One example to be fixed would be PaymentId, but there are others.
I think a PR focused on that would be better. I've created #2105

Could you label it good first issue?
@tnull
tnull
commented
Mar 15, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I think a PR focused on that would be better. I've created #2105 Could you label it good first issue?
SGTM, even though it might introduce some additional git noise.
TheBlueMatt
TheBlueMatt
reviewed
Mar 18, 2023
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Thanks, your cleanups turned up a bunch of places where the docs are just downright ancient and need updating :)
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/mod.rs
Outdated

@douglaz
douglaz
commented
Mar 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@TheBlueMatt Please double check the changes
TheBlueMatt
TheBlueMatt
reviewed
Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@douglaz
douglaz
commented
Mar 21, 2023

Copy link


Copy Markdown

ContributorAuthor
I think it's okay now 🤞
tnull
tnull
reviewed
Mar 21, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/mod.rs
Outdated

@douglaz
douglaz
commented
Mar 21, 2023

Copy link


Copy Markdown

ContributorAuthor
@tnull done
@TheBlueMatt
TheBlueMatt
merged commit ea15f0f
into
lightningdevkit:mainMar 21, 2023
@douglaz
douglaz
deleted the

docs_fixes

branch
March 21, 2023 18:55
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull approved these changes
@TheBlueMattTheBlueMattTheBlueMatt approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@douglaz@codecov-commenter@tnull@TheBlueMatt