gitlint: do not allow treewide as an area in commit messages #71091
Conversation
zephyrbot
added
area: Continuous Integration
area: Documentation
area: Coding Guidelines
nashif
added
Trivial
zephyrbot
requested review from
carlescufi,
fabiobaltieri,
jfischer-no,
kartben,
keith-zephyr and
stephanosio
nashif
force-pushed
the
topic/gitlint/treewide
branch
from
f094867 to
f355ae8
Compare
zephyrbot
added
area: Continuous Integration
area: Coding Guidelines
There was a problem hiding this comment.
not sure this is needed, using "treewide:" is no different from having a typo in the name of the subsystem/area or making up a new keyword that will never get used again, and we don't have checks for neither of this situations. I think having "good" prefixes eventually boils down to reviewers (and maintainer in particular) paying attention to the commit title and confirming it's to their liking for their area?
this is becoming a pattern in many PRs and you have to look closely to spot this. We do not have a preset list of allowed prefixes and I do not think we should have, detecting this and preventing "treewide" in the compliance check is still useful. |
Right, but isn't the real issue that "treewide: ..." on its own should be banned, but not e.g. "treewide: flash: ...". |
|
just re-read the suggested fix, I guess "flash: treewide: ..." would still be allowed |
| elif title.startswith('treewide'): | ||
| return [RuleViolation(self.id, violation_message, title)] |
There was a problem hiding this comment.
this is appending a fixed expression to what is otherwise a generic check (named like a specific one, but has a configuration https://github.com/zephyrproject-rtos/zephyr/blob/main/.gitlint#L28-L29 that allows extending well beyond "subsystem"), now the check naming and usage are misleading (see the error message)
I think this check should either be in its own gitlint class should be implemented in the existing config, but then the message should still be changed to reflect the new meaning
There was a problem hiding this comment.
the message is still valid: Commit title does not follow [subsystem]: [subject], treewide is not a subsystem.
Anyways, update to use previous regex.
nashif
force-pushed
the
topic/gitlint/treewide
branch
from
f355ae8 to
a0970cd
Compare
| @@ -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)" | |||
There was a problem hiding this comment.
I would also change the class name (TitleStartsWithSubsystem) and title (title-starts-with-subsystem) while at it, maybe something like TitleCheck or TitleFormat?
There was a problem hiding this comment.
why? TitleStartsWithSubsystem is correct and exact.
There was a problem hiding this comment.
because now it's TitleStartsWithSubsystemOrTreewide and how it's implement is more like "check the title against a regexp"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ok but then why keep
class TitleStartsWithSubsystem(LineRule):
name = "title-starts-with-subsystem"rathern than use a generic name?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
ok, I find it awkward that the implementation does the opposite but I guess it still kinda works, let's go with it
There was a problem hiding this comment.
I guess it depends on how you read it - "starts with [a] sub-system" vs. "starts with the literal 'subsystem'".
There was a problem hiding this comment.
yeah I realized that that's probably how it was meant to be read after Anas comment
Treewide changes touching a single area or topic should have the area/subsystem at the start of the commit message. Treewide is very ambigous. Signed-off-by: Anas Nashif <anas.nashif@intel.com>
nashif
force-pushed
the
topic/gitlint/treewide
branch
from
a0970cd to
2583b54
Compare
Treewide changes touching a single area or topic should have the
area/subsystem at the start of the commit message. Treewide is very
ambigous.
Signed-off-by: Anas Nashif anas.nashif@intel.com