Skip to content

support skipping linting #60

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

Closed
travi opened this issue Aug 14, 2017 · 17 comments
Closed

support skipping linting #60

travi opened this issue Aug 14, 2017 · 17 comments

Comments

@travi
Copy link
Contributor

travi commented Aug 14, 2017

with the transition from validate-commit-msg to commitlint, one missing feature that i notice is the ability specify that a commit should skip linting by including a keyword in the message. i initially asked if this would be added in the thread about the transition, but was righty directed here to open an issue.

in the case of validate-commit-msg, the keyword was simply starting the commit message with WIP, which has worked well for us when a commit is a step toward one of the categories, but does not complete the effort.

would it be possible to support similar functionality in commitlint to save us from needing to configure some sort of wrapper in all of our projects?

@marionebl
Copy link
Contributor

TL;DR

  • Git has the fixup! and squash! semantics
  • commitlint uses said semantics to determine ignored commits
  • I prefer sticking to git semantics to keep things simple
  • Let's discuss use cases that do not fit fixup! and squash! semantics
  • If we add this it should be configurable

The long story

commitlint currently skips commits according to git semantics - meaning that merge, fixup and squash commits are ignored automatically. The relevant test cases can be found here.

If i understand your use case for this correctly prefixing the WIP commits with fixup! instead could be the idiomatic way to ignore commits. E.g.:

  • WIP: This is something I id =>fixup! This is something I did
  • WIP: Some more work =>squash! Some more work

What are your thoughts on this?

Beginning commit messages with WIP feels different to me as git itself does not attach additional semantics to it. If we add this functionality I'd tend to go with some regex option matching commits that should be ignored, e.g.

module.exports = {
  rules: {},
  ignore: /(^WIP:|\[skip-lint\]$)/
};

@travi
Copy link
Contributor Author

travi commented Aug 14, 2017

thanks for the response. hopefully explaining our use case a bit more will clarify things. the main issue is that my team favors continuous integration over feature branches. we will very often merge commits into master that do not complete a feat or fix, but are instead steps toward one of those categories (and therefore do not fit the other categories).

thanks for pointing me to the existing ignore rules. i missed those when i was looking through things. i think those make a lot of sense for commits that target squashing later, but when the changes are in master before the remaining commits are completed, the squash approach doesn't fit. i think having fixup! or squash! commits would be odd if they weren't squashed together before merging into master.

i can totally get behind this going into the config as something like ignore, as you suggested. the current keyword works well for our team, but i see no reason to force it on everyone that way, especially since commitlint already provides a simple way to provide configuration.

other thoughts?

@marionebl
Copy link
Contributor

marionebl commented Aug 14, 2017

i think those make a lot of sense for commits that target squashing later, but when the changes are in master before the remaining commits are completed, the squash approach doesn't fit. i think having fixup! or squash! commits would be odd if they weren't squashed together before merging into master.

Understood. I assumed the WIP: commands would not end up in master. What's to to do:

  • Extend is-ignored to take an optional second parameter wildcard
  • Extend is-ignored.test
  • Add smoke test to load.test
  • Read new option from .ignores ignores should be an array of patterns to ignore.
  • Use matcher or comparable to match against commits

  • Bonus: Change current wildcards to use same matching mechanism

@marionebl
Copy link
Contributor

I threw together a rough todo list @travi. Can you help out on this?

@travi
Copy link
Contributor Author

travi commented Aug 15, 2017

sure, i'm up for helping move this forward. i have a few things i need to get off my plate before i can start checking things off the list though, so i wont be able to help out immediately

@travi
Copy link
Contributor Author

travi commented Aug 15, 2017

started to take a look at the change that would be necessary, and it doesnt seem like it will be too bad.

i think the only thing that i'm not clear about is the use of matcher rather than the current approach. it seems like the current approach would be fine as long as ignores is an array of regex patterns. are you looking to support multiple formats with this, or is this simply a change that you've wanted applied anyway and this is a good excuse to get it in?

@marionebl
Copy link
Contributor

marionebl commented Aug 15, 2017

sure, i'm up for helping move this forward. i have a few things i need to get off my plate before i can start checking things off the list though, so i wont be able to help out immediately

Thats nice, thanks. 🙇
Just ping me if you have more questions.

i think the only thing that i'm not clear about is the use of matcher rather than the current approach. it seems like the current approach would be fine as long as ignores is an array of regex patterns. are you looking to support multiple formats with this, or is this simply a change that you've wanted applied anyway and this is a good excuse to get it in?

The regular expressions are fine for internal API but I'd like to avoid them for userland configuration options. Glob patterns are easier than regular expressions and by taking an array of them users should be able to define almost any ignore rule they have in mind.

@travi
Copy link
Contributor Author

travi commented Sep 6, 2017

sorry to disappear for so long on this issue. i've been tinkering with a few different implementations, but i think i've landed on this being simply unnecessary for my particular use case.

i've realized that there are other linting capabilities offered by commitlint that weren't available from validate-commit-msg. since my intent of using wip was mostly to allow a commit that didnt fit the default categories, enabling the other linting capabilities would still be valuable.

since commitlint supports a shareable config, extending the allowed types is simple enough that i decided to go the route of simply adding wip as an allowed type in my config (if there is a simpler way to extend the list of types rather than replacing it, i'd love to understand).

since i have a solution for my use case, i probably won't find time to implement this capability at this point. i could still see some value in it for certain cases, but don't have a need for it currently. i'll leave the issue open so that you can decide if it is still functionality that you want, but feel free to close if not.

@cyberhck
Copy link

cyberhck commented Nov 8, 2017

there's always an option to setup git hooks on commit message (even easier now with npm package husky), commit, let this package lint. But when you want to skip, simply do a git commit -m '<msg>' --no-verify

@travi
Copy link
Contributor Author

travi commented Nov 9, 2017

the downside of --no-verify is that it skips all of the hooks. i use husky as well, but have both the commitmsg hook, but also the precommit hook where I run npm test. most of the time when I want to skip the linting of the commit message, i still want the precommit hook to run the other verification steps.

@cyberhck
Copy link

cyberhck commented Nov 9, 2017

Oh, I didn't realize that. Our policy is that we use angular commit message format and we are allowed to do WIP: ... commits, WIP aren't linted and nor are tested for anything, everything else is linted, and WIP are frowned upon, when you do WIP, you should be squashing with a proper commit. Helps keep evetrything clean.

@travi
Copy link
Contributor Author

travi commented Nov 10, 2017

sure, makes sense. i think that is a pretty common approach.

my team's use of WIP is a bit different, in order to encourage continuous integration. a change that is forward progress, but does not yet complete a feature or bug fix (and also does not make anything worse) should go ahead and go into master (and be deployed/published in our continuous deployment pipeline). therefore, we dont intend to squash those commits away and also intend for them to pass all normal validation.

essentially, we encourage WIP for "dark" release of changes until they are complete enough to be marked as feat or fix

@marionebl
Copy link
Contributor

Closing this for now as I feel the need for additional skipping mechanics is rather low. The recommended ways to make commitlint skip over commits are:

  • use fixup! and squash! commits
  • add WIP as allowed type to your configuration

@byCedric
Copy link
Member

byCedric commented Mar 2, 2018

I'm not sure if I should open a new issue or just leaving this in the same discussion. Right now I'm trying to get this into one of our Projects at work. We are using Bitbucket with the merge commit strategy to merge features/fixes/refactors etc into the develop branch. As of now, there is no way to customize the commit message automatically generated by Bitbucket. It also does not follow basic standards like using present tense words like Merge instead of Merged.

This is blocking us from implementing Commitlint and I can image other Bitbucket users are experiencing the same. It would be great if we can add one or more custom regexes of commits that should be ignored from the linter through the configuration file.

The exact (failing) error messages are these:

Merged in fix/eslint (pull request #17)
Merged in feature/fonts (pull request #14)
Merged in refactor/assets (pull request #2)
...

@marionebl
Copy link
Contributor

@byCedric Let's add the bitbucket format to the default ignore wildcards here: https://github.com/marionebl/commitlint/blob/master/%40commitlint/is-ignored/src/index.js

Can you send a PR for this?

@byCedric
Copy link
Member

byCedric commented Mar 2, 2018

@marionebl Sure, I will create one in my lunch break! Approximately in 1 or 2 hours. Thanks for responding this quickly.

bpedersen pushed a commit to bpedersen/commitlint that referenced this issue Oct 15, 2019
* fix: verify validation url and email

* chore: disable one expect

* fix: lint issue

* fix: fix lint
@jordan112
Copy link

jordan112 commented Feb 12, 2020

I solved this with the following:

ignores: [ (message) => message.includes('WIP') ]

https://stackoverflow.com/questions/60194822/how-do-you-configure-commitlint-to-ignore-certain-commit-messages-such-as-any-th#60195181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants