Skip to content
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.

Sign up for GitHub

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

gitlint: do not allow treewide as an area in commit messages #71091

Merged
merged 1 commit into from
Apr 10, 2024
Merged

gitlint: do not allow treewide as an area in commit messages #71091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitlint
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ extra-path=scripts/gitlint
# max-line-count=200

[title-starts-with-subsystem]
regex = ^(?!subsys:)(([^:]+):)(\s([^:]+):)*\s(.+)$
regex = ^(?!subsys:)(?!treewide:)(([^:]+):)(\s([^:]+):)*\s(.+)$

[title-must-not-contain-word]
# Comma-separated list of words that should not occur in the title. Matching is case
Expand Down
1 change: 1 addition & 0 deletions scripts/ci/twister_ignore.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,4 @@ scripts/checkpatch.pl
scripts/ci/pylintrc
scripts/footprint/*
scripts/set_assignees.py
scripts/gitlint/zephyr_commit_rules.py
2 changes: 1 addition & 1 deletion scripts/gitlint/zephyr_commit_rules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class TitleStartsWithSubsystem(LineRule):
def validate(self, title, _commit):
regex = self.options['regex'].value
pattern = re.compile(regex, re.UNICODE)
violation_message = "Commit title does not follow [subsystem]: [subject] (and should not start with literal subsys:)"
violation_message = "Commit title does not follow [subsystem]: [subject] (and should not start with literal subsys or treewide)"
Copy link
Member

@fabiobaltieri fabiobaltieri Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also change the class name (TitleStartsWithSubsystem) and title (title-starts-with-subsystem) while at it, maybe something like TitleCheck or TitleFormat?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? TitleStartsWithSubsystem is correct and exact.

Copy link
Member

@fabiobaltieri fabiobaltieri Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because now it's TitleStartsWithSubsystemOrTreewide and how it's implement is more like "check the title against a regexp"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it checks that you start with a : and verifies that some commonly made mistakes, i.e. use literal subsys and now treewide are not being made.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok but then why keep

class TitleStartsWithSubsystem(LineRule):
    name = "title-starts-with-subsystem"

rathern than use a generic name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok but then why keep

class TitleStartsWithSubsystem(LineRule):
    name = "title-starts-with-subsystem"

Because this check does nothing but verify that your title starts with a subsystem?

Copy link
Member

@fabiobaltieri fabiobaltieri Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I find it awkward that the implementation does the opposite but I guess it still kinda works, let's go with it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it depends on how you read it - "starts with [a] sub-system" vs. "starts with the literal 'subsystem'".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I realized that that's probably how it was meant to be read after Anas comment

if not pattern.search(title):
return [RuleViolation(self.id, violation_message, title)]

Expand Down
Loading