feat: support co-authorship lines in body#130
Conversation
|
@targos strangely, I can't tag people for review O.o cc @joyeecheung |
lib/rules/line-length.js
Outdated
| // Skip lines with URLs. | ||
| if (/https?:\/\//.test(line)) { continue } | ||
| // Skip co-authorship. | ||
| if (line.startsWith('Co-Authored-By')) { continue } |
There was a problem hiding this comment.
I think it would be more useful to properly validate the format (i.e. flag Co-authored-by: Author Name But No Email, etc.)
Also, could you add tests?
There was a problem hiding this comment.
Email is what causes the issue though
Co-authored-by: Michaël Zasso <targos@protonmail.com>
per existing `co-authored-by-is-trailer` rule's regex
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
test/rules/line-length.js
Outdated
| message: ` | ||
| fixup!: apply case-insensitive suggestion | ||
| Co-authored-by: Michaël Zasso <37011812+targos@users.noreply.github.com> | ||
| ` |
There was a problem hiding this comment.
This is not a good test, it passes without your patch because of the leading spaces
| message: ` | |
| fixup!: apply case-insensitive suggestion | |
| Co-authored-by: Michaël Zasso <37011812+targos@users.noreply.github.com> | |
| ` | |
| message: [ | |
| 'fixup!: apply case-insensitive suggestion', | |
| 'Co-authored-by: Michaël Zasso <37011812+targos@users.noreply.github.com>', | |
| ].join('\n') |
There was a problem hiding this comment.
It should fail though, no?
There was a problem hiding this comment.
A good regression test fails without the patch, and passes with it. Currently the test passes with and without your patch. If we want a good regression test (we do), we want one that fails without the patch.
There was a problem hiding this comment.
No, I mean it seems suspicious that it isn't failing, which would suggest the implementation is bugged.
There was a problem hiding this comment.
I discovered why the test is suspiciously passing:
core-validate-commit/lib/rules/line-length.js
Lines 32 to 33 in 39b93a3
| // Skip lines with URLs. | ||
| if (/https?:\/\//.test(line)) { continue } | ||
| // Skip co-authorship. | ||
| if (/^co-authored-by:/i.test(line)) { continue } |
There was a problem hiding this comment.
nit: I'm not sure it's worth doing, but we could have a shared util to avoid duplicating the regex
There was a problem hiding this comment.
🤔 I think if we need it in a third place, then it's worth having a shared util.
This comment was marked as off-topic.
This comment was marked as off-topic.
JakobJingleheimer
force-pushed
the
feat/support-coauthorship
branch
2 times, most recently
from
73cd024 to
8016e6a
Compare
JakobJingleheimer
force-pushed
the
feat/support-coauthorship
branch
from
8016e6a to
f05715d
Compare
4 participants
Co-authorship lines contain an email address which is very likely longer than 72 characters (any github-anonymised email address), which cannot be broken across multiple lines.