Skip to content

Add GLOSSARY.md and more #772

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 18, 2021
Merged

Add GLOSSARY.md and more #772

merged 2 commits into from
Jan 18, 2021

Conversation

ariard
Copy link

@ariard ariard commented Jan 9, 2021

This PR introduces a GLOSSARY.md to smooth future reviews contentious around what terms to use (#633). It also adds a Getting Project Context linking back to our Meeting Minutes for newcomers.

ariard
ariard commented Jan 9, 2021
View reviewed changes
CONTRIBUTING.md Outdated
is a good first step. You will learn who is working on what, how releases are drafed, what are the
pending tasks to deliver, where you can contribute review bandwidth, ...

A secondary step might to watch this talk on the LDK project (TODO: Val's Advancing Bitcoin).
Copy link
Author

Choose a reason for hiding this comment

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

@valentinewallace do you think we can link your Advancing Bitcoin presentation ? AFAIK that's the best video presenting LDK and it's not linked anywhere else ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Found the link: https://vimeo.com/showcase/7131712/video/418412286

Copy link
Author

Choose a reason for hiding this comment

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

I did remove this line from latest push, let's keep it for the LDK-website v0 or link it directly in README.md as you were suggesting yesterday during meeting ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, sounds good

@codecov
Copy link

codecov bot commented Jan 9, 2021 •
edited
Loading

Codecov Report

Merging #772 (3797fd0) into main (9c9c881) will decrease coverage by 0.12%.
The diff coverage is 88.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #772      +/-   ##
==========================================
- Coverage   91.35%   91.22%   -0.13%     
==========================================
  Files          37       37              
  Lines       22791    22866      +75     
==========================================
+ Hits        20821    20860      +39     
- Misses       1970     2006      +36
Impacted Files Coverage Δ
lightning/src/chain/keysinterface.rs 93.33% <0.00%> (-0.86%) ⬇️
lightning/src/lib.rs 100.00% <ø> (ø)
lightning/src/util/config.rs 50.00% <50.00%> (-19.05%) ⬇️
lightning/src/util/test_utils.rs 83.68% <66.66%> (-1.16%) ⬇️
lightning/src/ln/channel.rs 87.49% <86.95%> (-0.03%) ⬇️
lightning/src/chain/channelmonitor.rs 95.67% <100.00%> (+0.09%) ⬆️
lightning/src/ln/chanmon_update_fail_tests.rs 97.56% <100.00%> (ø)
lightning/src/ln/channelmanager.rs 84.94% <100.00%> (ø)
lightning/src/ln/functional_test_utils.rs 95.13% <100.00%> (ø)
lightning/src/ln/functional_tests.rs 96.91% <100.00%> (-0.27%) ⬇️
... and 3 more

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 9c9c881...2caf289. Read the comment docs.

devrandom
devrandom reviewed Jan 9, 2021
View reviewed changes
@devrandom
Copy link
Member

Would also be good to have a definition of "Initiator"

@ariard ariard force-pushed the 2021-01-getting-project-context branch from 498acb3 to 42582a7 Compare January 12, 2021 14:53
@ariard
Copy link
Author

ariard commented Jan 12, 2021

Updated at 42582a7. Thanks @devrandom @bmancini55 for reviews.

@jkczyz jkczyz self-requested a review January 12, 2021 16:13
devrandom
devrandom reviewed Jan 12, 2021
View reviewed changes
devrandom
devrandom approved these changes Jan 12, 2021
View reviewed changes
Copy link
Member

@devrandom devrandom left a comment

Choose a reason for hiding this comment

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

ACK (but see comment about "replicas")

jkczyz
jkczyz reviewed Jan 13, 2021
View reviewed changes
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.

Thanks for putting this together! Mainly nits but some questions around the terminology.

CONTRIBUTING.md Outdated
@@ -43,6 +43,15 @@ a "soft" commitment.
If you're eager to increase the velocity of the dev process, reviewing other contributors work is
the best you can do while waiting review on yours.

Getting Project Context
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if "context" best encapsulates all these ideas. They seem more about understanding developer or contributor interaction.

The word "context" is also used in the "Getting Start" section. Did you mean for these to mean the same thing? I think of context more in terms of understanding how the project is intended to be used.

Copy link
Author

Choose a reason for hiding this comment

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

I see, I do have a different understanding of context as : "Contributors practices and development process shaped by project history". But given your confusion, I think a better term is "Culture". Re-worded.

Your definition of "context" means more "Design Goals" to me :)

Copy link
Contributor

Choose a reason for hiding this comment

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

To a broader point, does this need to be separate section? There seems to be a lot of overlap between this and the "Getting Started" section.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah makes sense

@naumenkogs
Copy link
Contributor

Concept ACK.
Will go over it once you resolve suggestions from other reviewers.

@ariard ariard force-pushed the 2021-01-getting-project-context branch from 42582a7 to c9f42e6 Compare January 15, 2021 15:45
@ariard
Copy link
Author

ariard commented Jan 15, 2021

Thanks @jkczyz for the detailed review! I took more your minors but see non-resolved conversations for points who need a second look.

jkczyz
jkczyz reviewed Jan 15, 2021
View reviewed changes
TheBlueMatt
TheBlueMatt approved these changes Jan 15, 2021
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Happy with this modulo the comments Jeff had already left, so once Jeff's happy, I am too.

@ariard ariard force-pushed the 2021-01-getting-project-context branch from c9f42e6 to 2caf289 Compare January 18, 2021 16:07
@ariard
Copy link
Author

ariard commented Jan 18, 2021

Updated at 2caf289 with latest @jkczyz suggestions.

@TheBlueMatt TheBlueMatt merged commit 2ea3765 into lightningdevkit:main Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@bmancini55 bmancini55 bmancini55 left review comments

@valentinewallace valentinewallace valentinewallace left review comments

@devrandom devrandom devrandom approved these changes

@jkczyz jkczyz jkczyz 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.
7 participants
@ariard @devrandom @naumenkogs @TheBlueMatt @bmancini55 @jkczyz @valentinewallace