How should I deal with merged PRs that append an issue number, past 72 characters?

louh commented 3 years ago

I like the default header-max-length rule. Having a short commit message makes concise, readable commit logs and I don't want to lose that.

But let's say I make a commit like this:

chore(example): write commit message just under the 72 character limit

It's in another branch. I make a PR for this commit to my main branch. Our CI pipeline (using commitlint-travis) passes. We use the squash merge strategy instead of creating a merge commit so that the history continues to look clean.

Now there's a commit on our main branch, which appends an issue number. GitHub does this automatically for us.

chore(example): write commit message just under the 72 character limit (#1234)

CI runs again on the new commit on the main branch. This time it fails. The output log looks something like this.

$ commitlint-travis
⧗   input: chore(example): write commit message just under the 72 character limit (#1234)
✖   header must not be longer than 72 characters, current length is 78 [header-max-length]
✖   found 1 problems, 0 warnings
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

The command "commitlint-travis" exited with 1.

Expected Behavior

A PR that passes commitlint on Travis CI should not fail the build once merged.

Current Behavior

A PR can pass on Travis CI but fail once merged, because the default behavior of GitHub is to append an issue number to the commit, which can potentially increase the header length beyond the limit set by the header-max-length rule.

Possible Solution

Is it possible to do any of the following as potential solutions:

Override the header-max-length rule with a slightly higher limit depending on where the linter is run (e.g. Travis CI uses a higher limit but locally with husky does not)
Don't include issue numbers (syntax matching parenthesis and #number at the end of a header) in the max-length count

One "solution" may be to suggest that developers or code owners manually count the characters in every pull request title and shorten it themselves before merging, but I do not think this is something that can be enforced (if we could do it, we wouldn't need linters to begin with).

If there's any other solutions that have already been developed, please let me know.

Context

I will now link to the actual test runs from our real-world application so you can see this occurring in real life:

The passing PR: https://travis-ci.org/github/streetmix/streetmix/builds/746766308
The failing merged commit: https://travis-ci.org/github/streetmix/streetmix/builds/746806342

It would be great to be able to have a solution to this, because tests you don't expect to fail can seem scary. Only after clicking through would one realize what went wrong, and even then it's not clear why a commit message suddenly failed when it didn't before, unless you understood exactly what the rule is and what caused it to break.

Thanks!

escapedcat commented 3 years ago

Thanks for the great written issue!

I think max-length was related to the amount of chars a line starts breaking in certain situations. If this is true, adding a ticket-number will lead to that unwanted line-break as well. If my assumption is wrong, the ticket-number could simply be added automatically if this functionality would exist.
The current opinionated conventional-commits config is actually allowing 100 chars.
Would it be possible for you to use that one? Would that solve your issue? In certain cases you might run into the same problem. A solution for this would be a new feature I guess.

louh commented 3 years ago

Adopting the new config to raise the limit to 100 chars helps with this commit message, but the problem will just exist at the higher limit. A developer who makes a commit message at 99 characters, which passes, will see a failing build again just like this time when it is merged.

I think one possible solution that I would like is to treat the final issue number as a separate, and optional, component of a commit message. In the same way that a "scope" is optional and has a particular syntax and position in the header, an issue number can be defined without ambiguity:

It is separated from the end of the subject line by one space
It always takes this form: (#000) where 000 is an integer of arbitrary length
It always occurs at the end of a header line

When this component exists, exclude it from being counted against the header-max-length rule.

escapedcat commented 3 years ago

Adopting the new config to raise the limit to 100 chars helps with this commit message, but the problem will just exist at the higher limit. A developer who makes a commit message at 99 characters, which passes, will see a failing build again just like this time when it is merged.

Agreed, that's what i meant with: "In certain cases you might run into the same problem."

When this component exists, exclude it from being counted against the header-max-length rule.

Would this work nicely with the "enforcing a ticket number"-rule? In this case the ticket-number can be placed anywhere in the subject. Or would this be unrelated because in your case the CI would add the ticket-number? How should this behave if someone is using the "enforcing"-rule and this feature? I feel like this should be exclusive to each other, right? You either want one or the other.

louh commented 3 years ago

Yeah, good question. It seems the references (or ticket numbers) can be anywhere, whereas GitHub's specific implementation always adds in the same place. Furthermore the rule means that the developer making the commit is responsible for adding it, so they should still be subject to max-length rules.

I think in my mental model because GitHub is making the addition to the commit "automatically" it should just be ignored, as if it doesn't count at all toward commitlint rules.

I also think that this might need to be an opt-in thing.

I just discovered the section on plugins, would it be a good idea for me to explore this as a plugin first?

iamscottcab commented 3 years ago

I just discovered the section on plugins, would it be a good idea for me to explore this as a plugin first?

This was my first thought when I read it the other day. You might be able to make a custom plugin to run on your CI which parses the commit yourself (especially if the format never changes) and do a length test only on the commit message and not the ticket number.

louh commented 3 years ago

Here's my initial approach, using a local plugin. If it's a promising start, my next step would be to extract this into a standalone plugin so that it can be published to npm, have tests, etc.

The primary feature of this approach is that it shadows the original header-max-length rule, by replacing the parsed header with a truncated one (if the issue number is detected), but otherwise it works exactly the same as before.

// commitlint.config.js
const rules = require('@commitlint/rules');

module.exports = {
  extends: ['@commitlint/config-conventional'],
  plugins: [
    {
      rules: {
        'header-max-length': (parsed, _when, _value) => {
          parsed.header = parsed.header.replace(/\s\(#[0-9]+\)$/, '')
          return rules.default['header-max-length'](parsed, _when, _value)
        },
      },
    },
  ]
}

My main concern with this approach is that this may cause confusion because if the plugin is installed, it will create subtly different behavior than the documented header-max-length rule.

One way to resolve this is to put this under another name, e.g. header-max-length-gh-issue. In this case, the user will need to manually turn off the original header-max-length rule and turn this one on. This means they would also need to specify the max length value directly, something that would have been done for them otherwise if config-conventional (or other extendable presets) are turned on.

module.exports = {
  rules: {
    'header-max-length': [0],
    'header-max-length-gh-issue': [2, 'always', 100]
  }
}

Let me know what you think.

fharper commented 1 year ago

@louh plugin did the work for me, thanks 🎉

conventional-changelog / commitlint