Add authors-commit contrib rule #358

stauchert · 2022-11-03T08:04:44Z

It is possible to manage a so-called AUTHORS file in the repository. In this file, all committers for the respective project are listed.
With this gitlint rule, it can be ensured that only people who are listed in the AUTHORS file can commit.

Adds the ability to validate committers from an AUTHORS file when committing.

jorisroovers · 2022-11-07T09:17:21Z

gitlint-core/gitlint/tests/contrib/rules/test_authors_commit.py

+
+
+@pytest.fixture(name="git_ctx")
+def gen_git_ctx(tmpdir):


Please use the existing self.get_sample() fixture method. Example usage:

gitlint/gitlint-core/gitlint/tests/test_lint.py

Line 14 in 16145b3

gitcontext = self.gitcontext(self.get_sample("commit_message/sample1"))

jorisroovers · 2022-11-07T09:18:25Z

gitlint-core/gitlint/tests/contrib/rules/test_authors_commit.py

+
+@pytest.mark.parametrize("author", [AUTHOR_1, AUTHOR_2])
+def test_authors_succeeds(author, git_ctx):
+    assert not validate_authors_rule(author.name, author.email, git_ctx)


We should match existing test styling. While correct, we should stay consistent in the project and not use pytest style tests.
This includes not using @pytest.mark.parametrize, and not using assert statements.

gitlint-core/gitlint/tests/contrib/rules/test_authors_commit.py

jorisroovers · 2022-11-07T10:03:50Z

gitlint-core/gitlint/tests/contrib/rules/test_authors_commit.py

+from collections import namedtuple
+from unittest.mock import Mock, patch
+
+import pytest


Easiest is to not import pytest at all, that way we don't rely on pytest specific methods.

jorisroovers · 2022-11-07T10:07:18Z

gitlint-core/gitlint/contrib/rules/authors_commit.py

+    @classmethod
+    def _read_authors_from_file(cls, git_ctx) -> Tuple[str, str]:
+        for file_name in cls.authors_file_names:
+            path = Path(git_ctx.repository_path / file_name)


git_ctx.repository_path is still a string today. I agree it would be better if it were a Path, but that's legacy code that would need updating (different discussion).

File "/workspaces/gitlint/gitlint-core/gitlint/contrib/rules/authors_commit.py", line 37, in validate registered_authors, authors_file_name = Authors._read_authors_from_file(commit.message.context) File "/workspaces/gitlint/gitlint-core/gitlint/contrib/rules/authors_commit.py", line 24, in _read_authors_from_file path = Path(git_ctx.repository_path / file_name) TypeError: unsupported operand type(s) for /: 'str' and 'str'

This should now be fixed.

jorisroovers

Thanks for this PR, I think this makes sense to add as a contrib rule!

Notes:

Hit an exception when trying this out, see inline, should be easily fixed
Testing style would need updating to match existing project test style (i.e. don't import pytest), more details inline
Missing docs, would need to be added to https://github.com/jorisroovers/gitlint/blob/main/docs/contrib_rules.md

Appreciate your contribution!

jorisroovers

This looks really good!

2 nitpicks (also see inline):

Shall we call this rule contrib-allowed-authors? I think that's more descriptive and similar to other gitlint rule names. If so, gentle reminder to also update the docs (easily forgotten 😄 )
1 remaining case of using assert over assertEquals

Should be straight merge after this.

jorisroovers · 2022-11-10T08:45:44Z

gitlint-core/gitlint/contrib/rules/authors_commit.py

+    # A rule MUST have a human friendly name
+    name = "contrib-authors-commit"
+
+    # A rule MUST have a *unique* id, we recommend starting with UC (for User-defined Commit-rule).


This copy-paste comment from the examples is slightly confusing here since it references a User-defined rule. We should just remove it.

Suggested change

# A rule MUST have a *unique* id, we recommend starting with UC (for User-defined Commit-rule).

jorisroovers · 2022-11-10T08:45:50Z

gitlint-core/gitlint/contrib/rules/authors_commit.py

+    authors_file_names = ("AUTHORS", "AUTHORS.txt", "AUTHORS.md")
+    parse_authors = re.compile(r"^(?P<name>.*) <(?P<email>.*)>$", re.MULTILINE)
+
+    # A rule MUST have a human friendly name


Bit extraneous inside the gitlint codebase - suggest to remove

Suggested change

# A rule MUST have a human friendly name

jorisroovers · 2022-11-10T08:54:35Z

gitlint-core/gitlint/contrib/rules/authors_commit.py

+from gitlint.rules import CommitRule, RuleViolation
+
+
+class Authors(CommitRule):


Suggestion: AllowedAuthors is probably more descriptive.

Suggested change

class Authors(CommitRule):

class AllowedAuthors(CommitRule):

jorisroovers · 2022-11-10T08:55:23Z

gitlint-core/gitlint/contrib/rules/authors_commit.py

+    parse_authors = re.compile(r"^(?P<name>.*) <(?P<email>.*)>$", re.MULTILINE)
+
+    # A rule MUST have a human friendly name
+    name = "contrib-authors-commit"


Similar to class name, I think contrib-allowed-authors is probably more descriptive. Docs would need quick update too.

Suggested change

name = "contrib-authors-commit"

name = "contrib-allowed-authors"

jorisroovers · 2022-11-10T09:07:06Z

gitlint-core/gitlint/tests/contrib/rules/test_authors_commit.py

+        assert authors_file_name == "AUTHORS"
+        assert len(authors) == 1
+        assert authors == {self.author_1}


Suggested change

assert authors_file_name == "AUTHORS"

assert len(authors) == 1

assert authors == {self.author_1}

self.assertEqual(authors_file_name, "AUTHORS")

self.assertEqual(len(authors), 1)

self.assertEqual(authors, {self.author_1})

stauchert · 2022-11-10T12:47:05Z

Thanks for the note regarding the assert statement and the name suggestion! I agree with you on that. 👍

jorisroovers · 2022-11-11T11:00:39Z

Merged, super thanks!

- Python 3.11 support - Last release to support Python 3.6 - Behavior Change: In a future release, gitlint will be switching to use `re.search` instead of `re.match` semantics for all rules. (#254) - gitlint no longer uses the `sh` library by default in an attempt to reduce external dependencies. - `--commits` now also accepts a comma-separated list of commit hashes, making it possible to lint a list of non-contiguous commits without invoking gitlint multiple times (#283) - Improved handling of branches that have no commits (#188) - Support for `GITLINT_CONFIG` env variable (#189) - Added a new `gitlint-ci` pre-commit hook, making it easier to run gitlint through pre-commit in CI (#191) - Contrib Rules: - New `contrib-disallow-cleanup-commits` rule (#312) - New `contrib-allowed-authors` rule (#358) - User Defined rules: - Gitlint now recognizes `fixup=amend` commits, available as `commit.is_fixup_amend_commit=True` - Gitlint now parses diff **stat** information, available in `commit.changed_files_stats` (#314) - Bugfixes: - Use correct encoding when using `--msg-filename` parameter (#310) - Various documentation fixes (#244) (#263) (#266) (#294) (#295) (#347) (#364) - Under-the-hood: - Dependencies updated - Moved to blacked for formatting - Fixed nasty CI issue (#298) - Unit tests fix (#256) - Vagrant box removed in favor of github dev containers (#348) - Removed a few lingering references to the `master` branch in favor of `main` - Moved roadmap and project planning to github projects Full Release details in CHANGELOG.md.

Add authors-commit contrib rule

469c8f2

Adds the ability to validate committers from an AUTHORS file when committing.

jorisroovers reviewed Nov 7, 2022

View reviewed changes

gitlint-core/gitlint/tests/contrib/rules/test_authors_commit.py Show resolved Hide resolved

jorisroovers reviewed Nov 7, 2022

View reviewed changes

jorisroovers requested changes Nov 7, 2022

View reviewed changes

stauchert added 2 commits November 9, 2022 10:50

Update test styling

374a0cb

Add authors-commit contrib rule to documentation

1271c26

jorisroovers requested changes Nov 10, 2022

View reviewed changes

jorisroovers added this to the 0.18.0 milestone Nov 10, 2022

Update rule name

a51b25e

jorisroovers merged commit 3740e97 into jorisroovers:main Nov 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authors-commit contrib rule #358

Add authors-commit contrib rule #358

stauchert commented Nov 3, 2022

jorisroovers Nov 7, 2022 •

edited

jorisroovers Nov 7, 2022 •

edited

jorisroovers Nov 7, 2022

jorisroovers Nov 7, 2022 •

edited

stauchert Nov 9, 2022

jorisroovers left a comment

jorisroovers left a comment

jorisroovers Nov 10, 2022

jorisroovers Nov 10, 2022

jorisroovers Nov 10, 2022

jorisroovers Nov 10, 2022

jorisroovers Nov 10, 2022

stauchert commented Nov 10, 2022

jorisroovers commented Nov 11, 2022

		from gitlint.rules import CommitRule, RuleViolation


		class Authors(CommitRule):

	name = "contrib-authors-commit"
	name = "contrib-allowed-authors"

Add authors-commit contrib rule #358

Add authors-commit contrib rule #358

Conversation

stauchert commented Nov 3, 2022

jorisroovers Nov 7, 2022 • edited

Choose a reason for hiding this comment

jorisroovers Nov 7, 2022 • edited

Choose a reason for hiding this comment

jorisroovers Nov 7, 2022

Choose a reason for hiding this comment

jorisroovers Nov 7, 2022 • edited

Choose a reason for hiding this comment

stauchert Nov 9, 2022

Choose a reason for hiding this comment

jorisroovers left a comment

Choose a reason for hiding this comment

jorisroovers left a comment

Choose a reason for hiding this comment

jorisroovers Nov 10, 2022

Choose a reason for hiding this comment

jorisroovers Nov 10, 2022

Choose a reason for hiding this comment

jorisroovers Nov 10, 2022

Choose a reason for hiding this comment

jorisroovers Nov 10, 2022

Choose a reason for hiding this comment

jorisroovers Nov 10, 2022

Choose a reason for hiding this comment

stauchert commented Nov 10, 2022

jorisroovers commented Nov 11, 2022

jorisroovers Nov 7, 2022 •

edited

jorisroovers Nov 7, 2022 •

edited

jorisroovers Nov 7, 2022 •

edited