Skip to content

Conversation

@ariard
Copy link

@ariard ariard commented Nov 2, 2020

Our current protocol dev onboarding is a work-in-progress, having a least an explicit "Getting Started" is really needed at its this phase of the project.

@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #745 (3e20367) into main (9c7c3b9) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #745      +/-   ##
==========================================
- Coverage   91.38%   91.37%   -0.01%     
==========================================
  Files          37       37              
  Lines       22067    22067              
==========================================
- Hits        20165    20164       -1     
- Misses       1902     1903       +1     
Impacted Files Coverage Δ
lightning/src/ln/functional_tests.rs 96.99% <0.00%> (-0.04%) ⬇️
lightning/src/util/ser.rs 87.78% <0.00%> (+0.32%) ⬆️

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 9c7c3b9...7e7635d. Read the comment docs.

@ariard
Copy link
Author

ariard commented Nov 3, 2020

Thanks @moneyball for the fixes, took them directly on my commit with credits.

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.

Overall, looks good! Just some minor nits.

CONTRIBUTING.md Outdated
development compared to your merged work.

Even if you have an extensive open source background or sound software engineering skills, consider
that comprehension by the other set of regular contributors is as much important than its technical
Copy link
Contributor

Choose a reason for hiding this comment

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

It's ambiguous as to what "its" is referring to. Should it say is?

Copy link
Author

Choose a reason for hiding this comment

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

Corrected, added code before comprehension.

CONTRIBUTING.md Outdated
development compared to your merged work.

Even if you have an extensive open source background or sound software engineering skills, consider
that comprehension by the other set of regular contributors is as much important than its technical
Copy link
Contributor

Choose a reason for hiding this comment

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

s/than/as

CONTRIBUTING.md Outdated
correctness.

It's very welcome to ask for review, either on IRC or LDK Slack. And also for reviewers, it's nice
to provide timelines when you hope to fulfill the request while bearing in mind for both side that's
Copy link
Contributor

Choose a reason for hiding this comment

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

s/side/sides

@ariard
Copy link
Author

ariard commented Nov 6, 2020

Thanks for review, updated at 039ffee.

CONTRIBUTING.md Outdated
development compared to your merged work.

Even if you have an extensive open source background or sound software engineering skills, consider
that code comprehension by the other set of regular contributors is as much important as its
Copy link
Contributor

Choose a reason for hiding this comment

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

Would recommend rephrasing "the other set of regular contributors" simply as "the reviewers". Tightening it up makes the connection between "comprehension" and "technical correctness" more apparent without loss of meaning.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, updated at 1dfb2f9

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about nit picking this to death. After reading the new version, I think the following wording would be clearer:

that the reviewers' comprehension of the code is as much important as technical correctness.

Copy link
Author

Choose a reason for hiding this comment

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

That's a minute update, that's fine :)

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 7e7635d

@TheBlueMatt TheBlueMatt merged commit 3aa0253 into lightningdevkit:main Nov 12, 2020
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.

3 participants