Skip to content
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

doc: CONTRIBUTING.md improvements #19494

Merged
merged 1 commit into from
Jul 14, 2020
Merged

doc: CONTRIBUTING.md improvements #19494

merged 1 commit into from
Jul 14, 2020

Conversation

jonatack
Copy link
Member

The motivation here was to add a mention of hygienic commits following a discussion today, e.g. something along the lines of:

Make sure each individual commit is hygienic, building successfully on its own without warnings, errors, or regressions, and that all tests pass.

While here, made various fixups. They are optional and can be omitted.

@jonatack
Copy link
Member Author

Friendly ping to Bitcoin's outstanding technical writer, @harding.

practicalswift
practicalswift reviewed Jul 11, 2020
View reviewed changes
Copy link
Contributor

@practicalswift practicalswift left a comment

Choose a reason for hiding this comment

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

Concept ACK

CONTRIBUTING.md Outdated Show resolved Hide resolved
hebasto
hebasto reviewed Jul 11, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Concept ACK.

How about adding a requirement to provide a relevant prefix to a commit message as it done for PR title?

@DrahtBot DrahtBot added the Docs label Jul 11, 2020
harding
harding approved these changes Jul 11, 2020
View reviewed changes
Copy link
Contributor

@harding harding left a comment

Choose a reason for hiding this comment

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

LGTM. A few comments, all nits (so ignore anything you think is irrelevant). Thanks!

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
practicalswift
practicalswift reviewed Jul 11, 2020
View reviewed changes
CONTRIBUTING.md Outdated Show resolved Hide resolved
@jonatack jonatack force-pushed the contributing-md-improvements branch from 680e152 to b03697b Compare July 12, 2020 05:53
@jonatack
Copy link
Member Author

Thanks @practicalswift, @hebasto and @harding! Excellent feedback, all added to the changes (except commit message prefixes, as I am not sure it's seen as a requirement -- happy to update depending on feedback).

@harding
Copy link
Contributor

harding commented Jul 13, 2020

ACK b03697b Locally reviewed the word diff.

@maflcko
Copy link
Member

maflcko commented Jul 13, 2020

ACK b03697b 🚌

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK b03697b68e24bea7a177f84954c93691450d5638 🚌
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjiAwwArYzmdxwSe30IGTcmetpjGOqS+qwyh6D4q4CZaJRYWtoGqRdf/4/6+vr2
w7mt+TeoMc26VH3WFbyQwlZ9ybyJ4FtiVLigbqcymIBJDCTS3lrF6JpQAsgwDAj7
fH8qeI7/EcxcAnixLy9iVEFcFeUlGxzhMzzUcoJIB+xSg1g9oBtFxGw51HrFv4dy
IvFJY6ROtiuh8LwsivBumjGktt6KZRvzGnIcKDLdu2vEKXH6Y7pItlETDcHUH6Sr
0mWHLqFZfM8UKhtSGlgOo5aauLdTbC+FUmUvz8G5ch6YNGONdBYXcxiFyhn+U5r/
TmNBIRmnwmqh0FOYzPVOsEDq4DM48plZFphDh3t+LC5K4Qdf0nNwfkFTsy4MfJsN
5KlTl5r1O+2EkSVf89wj5kCoaszZHJP8rM3b+JkaIURQ9QQT266SsdY/sBWoqhAb
OHK8ndQV3q9pJ2C4gjUi9SiIzp7KEg1hnNRxm/jl6j3/0AVvJAcE+wpMMoK1fvGY
VLUE/uPA
=A2g7
-----END PGP SIGNATURE-----

Timestamp of file with hash e10175f73b1b900e2608a1f748db725df303ef08fffac41b83511625d6971544 -

hebasto
hebasto approved these changes Jul 13, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK b03697b, I have reviewed the changes and they look OK, I agree they can be merged.

@practicalswift
Copy link
Contributor

ACK b03697b

@laanwj laanwj merged commit 89fa736 into bitcoin:master Jul 14, 2020
@jonatack jonatack deleted the contributing-md-improvements branch July 14, 2020 03:34
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@practicalswift practicalswift practicalswift left review comments

@harding harding harding approved these changes

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
Docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jonatack @harding @maflcko @practicalswift @hebasto @laanwj @DrahtBot