Add regex for commit title that skips body checks, if matches #57

SteffenKockel · 2018-03-05T14:27:01Z

We needed a possibility to exclude release commits from our rigorous gitlint-checks.

This pull requests has the following to offer

An option to specify a regular expression and, if it matches on the commit messages title, skip further body checks for the given commit.
An option to set the loglevel from commandline. This behavior was splitted between -d and -v[vvv]

bdrung

A new test case would be nice.

bdrung · 2018-03-05T15:45:18Z

gitlint/files/gitlint

@@ -44,6 +44,8 @@

 # [body-min-length]
 # min-length=5
+# ignore-body-checks-regex=''
+


unnecessary empty line

bdrung · 2018-03-05T15:46:08Z

gitlint/rules.py

@@ -79,14 +81,14 @@ def __eq__(self, other):
        return equal

    def __str__(self):
-        return sstr(self)  # pragma: no cover
+        return self.__unicode__()  # pragma: no cover


Why do you use self.__unicode__()? Does this work with Python 2 too?

Yes, this works also with python 2 (Tested on python 2.7.14)

bdrung · 2018-03-05T15:46:39Z

gitlint/rules.py

@@ -243,14 +245,23 @@ def validate(self, commit):
            if first_line != "":
                return [RuleViolation(self.id, "Second line is not empty", first_line, 2)]

-


Please restore the newline. Otherwise pep8 will complain.

bdrung · 2018-03-05T15:47:30Z

gitlint/rules.py


    def validate(self, commit):
        min_length = self.options['min-length'].value
+        regex = self.options['ignore-body-checks-regex'].value
+        regex_valid = True if (self.options['ignore-body-checks-regex'].value) else False


regex_valid is not needed (see below).

bdrung · 2018-03-05T15:48:01Z

gitlint/rules.py

+        regex = self.options['ignore-body-checks-regex'].value
+        regex_valid = True if (self.options['ignore-body-checks-regex'].value) else False
+
+        if regex_valid and re.search(regex, commit.message.title):


You can directly check for regex:

if regex and re.search(regex, commit.message.title):

bdrung · 2018-03-05T15:50:10Z

gitlint/rules.py


    def validate(self, commit):
+        regex = self.options['ignore-body-checks-regex'].value
+        regex_valid = True if (self.options['ignore-body-checks-regex'].value) else False


Same as above.

bdrung · 2018-03-05T15:50:35Z

gitlint/rules.py

-    options_spec = [BoolOption('ignore-merge-commits', True, "Ignore merge commits")]
+    # This needs to come from the real options
+    options_spec = [BoolOption('ignore-merge-commits', True, "Ignore merge commits"),
+                    StrOption('ignore-body-checks-regex', '^Release', 'Ignore body checks for tiles matching regex')]


The default of ignore-body-checks-regex should be empty.

bdrung · 2018-03-05T15:51:11Z

gitlint/rules.py

+        if regex_valid and re.search(regex, commit.message.title):
+            logmsg = "Skipping %s (%s), because ignore-body-checks-regex matches." % (self.id, self.name)
+            LOG.info(logmsg)
+            return []


To be consistent, just return nothing.

bdrung · 2018-03-05T15:51:35Z

gitlint/tests/test_body_rules.py

@@ -77,12 +77,12 @@ def test_body_min_length(self):
        commit = self.gitcommit("Title\n\nThis is the second body line\n")

        violations = rule.validate(commit)
-        self.assertIsNone(violations)
+        self.assertIn(violations, [[],None])


This change is not needed if you return None.

bdrung · 2018-03-05T15:51:40Z

gitlint/tests/test_body_rules.py


        # assert no error - no body
        commit = self.gitcommit(u"Tïtle\n")
        violations = rule.validate(commit)
-        self.assertIsNone(violations)
+        self.assertIn(violations, [[], None])


This change is not needed if you return None.

bdrung · 2018-03-06T11:31:03Z

gitlint/cli.py

@@ -122,14 +122,15 @@ def stdin_has_data():
              help="Verbosity, more v's for more verbose output (e.g.: -v, -vv, -vvv). [default: -vvv]", )
 @click.option('-s', '--silent', help="Silent mode (no output). Takes precedence over -v, -vv, -vvv.", is_flag=True)
 @click.option('-d', '--debug', help="Enable debugging output.", is_flag=True)


--debug should be either removed or mapped to loglevel=debug.

This breaks even more tests and is not what we want to achieve here. Rejected.

bdrung · 2018-03-06T11:32:00Z

gitlint/tests/test_body_rules.py

+        commit = self.gitcommit(u"Release foo-bar-baz 0.12.3")
+        violations = rule.validate(commit)
+        self.assertIsNone(violations)
+


That test should go into a separate test method.

The benefit of separate test cases is that it easier to spot which part breaks and it prevents hiding failures (the test will abort on the first assertion error).

bdrung · 2018-03-06T11:32:05Z

gitlint/tests/test_body_rules.py

+        commit = self.gitcommit(u"Release foo-bar-baz 0.12.3")
+        violations = rule.validate(commit)
+        self.assertIsNone(violations)
+


That test should go into a separate test method.

Some commits don't need a body. This adds an option (B5.ignore-body-checks-regex) to specify a regex. If this regex matches commit.message.title, further body checks are skipped. refs #39774 Signed-off-by: Steffen Kockel <steffen.kockel@profitbricks.com>

Signed-off-by: Steffen Kockel <steffen.kockel@profitbricks.com>

jorisroovers · 2018-03-30T09:21:09Z

It doesn't entirely sit right with me is that this is being implemented as an option for specific rules.

IMO the solution should be generic.

More something along the lines of:

[ignore.title-match]
regex=^Release(.*)
ignore=T1,body-min-length,B6
# Note that like with general ignore, ignore accepts both names and ids

[ignore.body-line-match]
regex=^Hotfix
ignore=all

# This can be added later
[ignore.meta-author-match]
regex=bossman@company.com
ignore=all

There's several hours of work involved with implementing this I think (and like usually, the majority of time spent on tests and docs).

Interested to hear your thoughts.

Let me know if you intend on working on this, if not, I can probably look at it at some point (today, next month, later this year, really depends on when I find/make time).

jorisroovers · 2018-03-30T14:50:36Z

Put some more thought into this, I think I've got a better solution.

We can introduce a new type of rule IgnoreRule that are ran before other rules and when they match, gitlint will stop running other rules for that particular commit.

The nice thing about this approach is that it's generic and that other users can easily extend it by building their own custom IgnoreRules - similarly to the existing functionality for building user-defined rules.

Gitlint would then just implement a couple of built-in ignore-rules that are set to ignore nothing by default, but can be configured like so:

[ignore-by-title]
regex="^Release(.*)"

[ignore-by-body-line]
regex="^Hotfix(.*)"

I'll work on this :-)

Very much WIP, still need to write/fix tests, docs, etc. This is part of #54 and #57.

Added a new IgnoreByTitle rule that is based on a new more generic rule construct called ConfigurationRule. ConfigurationRules can modify gitlint configuration (and therefore behavior) on a per commit basis. Added some tests and docs, more docs and integration tests to follow in additional commits. This is part of #54 and #57.

jorisroovers · 2018-04-01T14:29:28Z

This was implemented as part of 7d44795 and 52dc67e.

Will be released fairly soon as part of v0.10.0. Closing this out.

SteffenKockel · 2018-04-05T17:03:00Z

This was actually my first contact with this code. Thanks for incorporating the requested feature. No bad feelings here. 👍

bdrung suggested changes Mar 5, 2018

View reviewed changes

SteffenKockel changed the title ~~Dear Maintainer~~ Add regex for commit title that skips body checks, if matches Mar 5, 2018

bdrung reviewed Mar 6, 2018

View reviewed changes

bdrung suggested changes Mar 6, 2018

View reviewed changes

Steffen Kockel added 2 commits March 6, 2018 17:07

Fixup: Fix tests for ignore-body-checks-syntax

634b9c0

Signed-off-by: Steffen Kockel <steffen.kockel@profitbricks.com>

jorisroovers mentioned this pull request Mar 29, 2018

Ignoring commits based on rules #54

Closed

jorisroovers added the enhancement User-facing feature enhancements label Mar 30, 2018

jorisroovers self-assigned this Mar 30, 2018

jorisroovers pushed a commit that referenced this pull request Mar 30, 2018

Ignoring commits based on rules

672d041

Very much WIP, still need to write/fix tests, docs, etc. This is part of #54 and #57.

jorisroovers closed this Apr 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add regex for commit title that skips body checks, if matches #57

Add regex for commit title that skips body checks, if matches #57

SteffenKockel commented Mar 5, 2018

bdrung left a comment

bdrung Mar 5, 2018

bdrung Mar 5, 2018

SteffenKockel Mar 6, 2018

bdrung Mar 5, 2018

bdrung Mar 5, 2018

bdrung Mar 5, 2018

bdrung Mar 5, 2018

bdrung Mar 5, 2018

bdrung Mar 5, 2018

bdrung Mar 5, 2018

bdrung Mar 5, 2018

bdrung Mar 6, 2018

SteffenKockel Mar 6, 2018

bdrung Mar 6, 2018

SteffenKockel Mar 6, 2018 •

edited

Loading

bdrung Mar 6, 2018

bdrung Mar 6, 2018

SteffenKockel Mar 6, 2018

jorisroovers commented Mar 30, 2018

jorisroovers commented Mar 30, 2018

jorisroovers commented Apr 1, 2018

SteffenKockel commented Apr 5, 2018

		@@ -243,14 +245,23 @@ def validate(self, commit):
		if first_line != "":
		return [RuleViolation(self.id, "Second line is not empty", first_line, 2)]

Add regex for commit title that skips body checks, if matches #57

Add regex for commit title that skips body checks, if matches #57

Conversation

SteffenKockel commented Mar 5, 2018

bdrung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteffenKockel Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisroovers commented Mar 30, 2018

jorisroovers commented Mar 30, 2018

jorisroovers commented Apr 1, 2018

SteffenKockel commented Apr 5, 2018

SteffenKockel Mar 6, 2018 •

edited

Loading