feat(rules): make body-max-line-length ignore lines with URLs #4486
Conversation
Previously the multi-line test fixtures in the test suite for the `body-max-line-length` rule were defined but never used; instead, the single line fixtures were being used in tests supposed to test multi-line commit messages. This was presumably an innocent copy-and-paste error, so fix the multi-line tests to use the multi-line fixtures.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
Can you also please do it for the footer-max-line-length? I've noticed that commitlint was complaining about my long URLs because it was thinking those were part of the footer. So in my application I've changed both the body and the footer rules to just warning instead of error. (I haven't even tried your PR. I'm just talking from my experience as someone working on a project that has commitlint enabled.) |
There was a problem hiding this comment.
While I have been a proponent of a more complex solution, I would like to commend you, @aspiers, for digging into this and doing what many (myself included) did not - submitting an actual solution! Thank you for looking into this! I'm hoping it will be merged and am looking forward to make use of the changes.
Note for other reviewers: I've only made a passing read-through on my phone - actual code owners should not rely on my review as complete.
| // | ||
| // This is overly lenient, in order to avoid costly regexps which | ||
| // have to worry about all the many edge cases of valid URLs. | ||
| const URL_REGEX = /\bhttps?:\/\/\S+\.\S+/; |
There was a problem hiding this comment.
I wonder if it would be possible to support generalised URIs (i.e. other URI schemes than http(s) as well, e.g. to allow ftp:// or urn:isbn: style URIs?
I suppose this would require an even more permissive regexp which may not be desired (such as \S+:\S+). An alternative would be to support only the official IANA registered schemes (e.g. (https?|ftp|urn)).
There was a problem hiding this comment.
\S+:\S+ would indeed be way too permissive, and result in permitting all kinds of long lines which do not contain genuine URIs.
The list of official IANA registered schemes is extremely long and constantly changing, so I don't see that as a practical way forward either. Plus for many URI schemes there is no good reason for them to be super long, e.g. mailto:, tel:, and presumably urn:isbn: too.
We could use a small hardcoded list of the most popular protocols, e.g. /https?|ftp|file/ but I think anything much more complex than this is allowing the quest for perfection to become the enemy of good enough. I'm very confident that the vast majority of cases where this rule is currently getting in the way are all related to https URLs.
| // | ||
| // This is overly lenient, in order to avoid costly regexps which | ||
| // have to worry about all the many edge cases of valid URLs. | ||
| const URL_REGEX = /\bhttps?:\/\/\S+\.\S+/; |
There was a problem hiding this comment.
I'd suggest dropping the trailing \.\S+ part of the regex. I don't see the relevance and it will exclude URLs that use a single hostname, e.g. http://localhost/hello/world.
There was a problem hiding this comment.
I do wonder how often a localhost URL needed in a commit message is likely to be super long, but OK I can imagine this happening as a corner case so I can change that.
There was a problem hiding this comment.
Dropped \.\S+ and force-pushed so this is now resolved.
There was a problem hiding this comment.
I do wonder how often a localhost URL needed in a commit message is likely to be super long
Manufactured example that reflects real-life:
This commits fixes a bug in the page foobar when the parameters are fizzbuzz.
http://localhost/application/management/foo/bar-manager?foo=12345678&bar=12345678&fizz=12345678&buzz=12345678
Adding URLs to the localhost dev server can be a simple way to explain how to reproduce a certain bug or how to access a certain new feature.
There was a problem hiding this comment.
Yes, that's exactly what I was imagining. Like I said, I suspect it's a rare corner case that the combination of the path and query params are long enough to trigger the rule. But anyway, this case is now covered just in case it happens.
As per my above comment, please can you specify exactly which ones you think need to be supported? |
|
It just occurred to me that But again, I think that worrying about these corner cases inside this PR is probably a mistake, because they can be dealt with incrementally in later refinements. This is preferable to further delaying a fix to what we already know is by far the most common problem, i.e. commit messages which want to refer to external web pages with long |
Whilst the `body-max-line-length` is almost always desirable, there is one fairly frequent scenario in which it gets in the way: when lines contain URLs exceeding the maximum line length. In this case, the URL cannot be split across lines without breaking it, and minifying it results in obfuscation and a dependency on a third-party service, both of which are undesirable. So make an exception in this rule for any line which contains a URL. Allow other content on the same line, in order to support use of various rich-text formats such as Markdown, which are often used in order to render links in more elegant ways.
aspiers
force-pushed
the
body-max-line-length-URLs-exception
branch
from
c7dc1e6 to
13a7b75
Compare
|
I think only including |
Description
Whilst the
body-max-line-lengthis almost always desirable, there is one fairly frequent scenario in which it gets in the way: when lines in the body of the commit message contain URLs exceeding the maximum line length.In this case, the URL cannot be split across lines without breaking it, and minifying it results in obfuscation and a dependency on a third-party service, both of which are undesirable.
So make an exception in this rule for any line which contains a URL. Allow other content on the same line, in order to support use of various rich-text formats such as Markdown, which are often used in order to render links in more elegant ways.
Motivation and Context
After lengthy discussion in #2112 of how to handle long URLs, this PR proposes a simple fix which seems to be closest to achieving broad consensus as a solid first step. More sophisticated refinements could potentially be layered on later, although there is not yet clear consensus what those might be.
Usage examples
How Has This Been Tested?
Relevant unit tests have been added, and it's been tested from the CLI as shown above.
Types of changes
Technically this is a change of existing functionality, although in practice it's so minor that it's more like a new feature.
Checklist: