feat: strip prefix #397
feat: strip prefix #397
Conversation
If fixups are allowed, then the remainder of the commit message ought to either be either a previous commit's subject line, or a hash: > If a commit message starts with "squash! ", "fixup! " or "amend! ", the remainder of the subject line is taken as a commit specifier, which matches a previous commit if it matches the subject line or the hash of that commit. So we can either: 1. Assume that previous commit's subject line was previously validated and do nothing further; or, 2. Validate the remainder of the commit's subject line as is. This specific implementation opts for the latter. Signed-off-by: JP-Ellis <josh@jpellis.me>
| @@ -326,6 +327,14 @@ pub(crate) fn check_fixup( | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) fn strip_fixup(message: &str) -> &str { | |||
| if let Some(message) = message.strip_prefix("fixup! ") { | |||
There was a problem hiding this comment.
This has a semantic conflict with #395
There was a problem hiding this comment.
I'm sorry, I was targeting the main branch when creating this, and therefore ensured that semantics are consistent with the target. It would be non-atomic to have the work on this PR rely on another as-yet unmerged PR.
There was a problem hiding this comment.
Posting multiple PRs that will conflict when merged in non-obvious ways is not helpful and is depending on the maintainer to catch something that was known.
In a case like this, you can always wait.
There was a problem hiding this comment.
While I understand that this PR has a semantic conflict with #395, I have had no indication of whether #395 will be merged. Therefore, it seemed best to ensure this PR aligns with the current state of the master branch.
In my experience, it's not uncommon to have PRs that may need adjustments as other related PRs are merged. This ensures that each change is reviewed and integrated based on its own merits and the current state of the target branch. Furthermore, having a PR be created for visibility is, in my experience, better than relying on contributors waiting before adding further improvements.
I also split up my PRs based on your earlier feedback. The boundary between "atomic" commits and PRs can sometimes be nuanced. The functionality to strip the prefix is distinct from the changes introduced in #395, which is why they were separated.
If or when it comes to merging this PR in due time, the necessary adjustments can be made based on the state of the master branch at that time.
There was a problem hiding this comment.
In my experience, it's not uncommon to have PRs that may need adjustments as other related PRs are merged.
I understand that and don't have a problem with it. What I am concerned about is the lack of communication on that front. The relationship of the PRs needs to be made clear.
Furthermore, having a PR be created for visibility is, in my experience, better than relying on contributors waiting before adding further improvements.
There are multiple ways of handling that. One is splitting where you can, like you did here. The other is communicating it. You did that by talking about this in the issue and you can foreshadow that in a PR description.
Pull Request Test Coverage Report for Build 10936396451Details
💛 - Coveralls |
If fixups are allowed, then the remainder of the commit message ought to either be either a previous commit's subject line, or a hash:
So we can either:
This specific implementation opts for the latter.
Resolves #392