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.

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

docs: Document the process for merging pull requests #5010

Merged
merged 9 commits into from
Jul 31, 2024

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented May 1, 2024

High Level Overview of Change

Adds instructions to CONTRIBUTING.md for creating and merging PRs, and for creating and merging releases.

Context of Change

This documentation arose out of the post-mortem when creating a new beta took an excessively long time.

Type of Change

  • Documentation update

Future Tasks

The PR/merge process may still have room for improvement in the future.

@codecov-commenter
Copy link

codecov-commenter commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.4%. Comparing base (a39720e) to head (fa786b3).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5010     +/-   ##
=========================================
- Coverage     71.4%   71.4%   -0.0%     
=========================================
  Files          796     796             
  Lines        67042   67042             
  Branches     10863   10867      +4     
=========================================
- Hits         47844   47839      -5     
- Misses       19198   19203      +5     

see 5 files with indirect coverage changes

Impacted file tree graph

Copy link
Collaborator

@intelliot intelliot left a comment

Choose a reason for hiding this comment

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

i think this is a good improvement over what we currently have, and we can always iterate/modify this with future PRs

@ximinez ximinez marked this pull request as ready for review July 9, 2024 17:49
@ximinez
Copy link
Collaborator Author

ximinez commented Jul 9, 2024

I squashed the previous changes, and marked the PR as no longer a draft. The first commit is the exact set of changes from prior. The second makes a couple of tweaks.

@ximinez ximinez requested a review from Bronek July 9, 2024 17:54
Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

Minor comments only

CONTRIBUTING.md Outdated
Comment on lines 76 to 81
Changes should be usually squashed down into one commit with a
[good commit message](#good-commit-messages).
In some cases, it is appropriate to organize the changes into
multiple commits. In that situation, all of the commits should be
distinct changes and have
[good commit messages](#good-commit-messages).
Copy link
Collaborator

Choose a reason for hiding this comment

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

A non-trivial PR will have more than one commit for a good reason, so I wouldn't say "should be usually squashed into one commit"; instead I suggest something like this:

The purpose of separation of a change into commits is not to reflect the history of how the author implemented the change "warts and all", instead it is to describe the change in smaller (yet well-formed) chunks, that make most sense to the reader. For this reason:

  • changes should be presented in a logical sequence of a small number (ideally one, for a small change) of commits
  • each commit should have a good message to explain a specific aspects of the change.
  • every commit should be signed
  • every commit should be well-formed (build passing, unit tests passing), as this helps to resolve merge conflicts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that there could be good reasons for multiple commits, but I think that should be an uncommon exception. Most PRs should be opened with a single squashed commit.

I have rewritten this part to reflect the rest of your suggestions.

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
ximinez added 5 commits July 19, 2024 17:09
* upstream/develop:
  Add xrpld build option and Conan package test (5052)
* upstream/develop:
  Update BUILD.md after PR 5052 (5067)
* upstream/develop:
  chore: Add comments to SignerEntries.h (5059)
  chore: Rename two files from Directory* to Dir*: (5058)
* upstream/develop:
  Ensure levelization sorting is ASCII-order across platforms (5072)
  fix: Fix NuDB build error via Conan patch (5061)
  Disallow filtering account_objects by unsupported types (5056)
Copy link
Collaborator

@Bronek Bronek 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 writing this !

@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jul 30, 2024
@ximinez ximinez assigned ximinez and unassigned Bronek Jul 30, 2024
@ximinez ximinez merged commit f5a3495 into XRPLF:develop Jul 31, 2024
21 of 22 checks passed
@ximinez ximinez deleted the pr/merging branch July 31, 2024 00:19
@ximinez ximinez mentioned this pull request Jul 31, 2024
1 task
vlntb pushed a commit to vlntb/rippled that referenced this pull request Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants