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.

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

New Rule: title-min-length #140

Merged
merged 5 commits into from
Oct 18, 2020

Conversation

mrshu
Copy link
Contributor

@mrshu mrshu commented Aug 8, 2020

  • Add a new title-min-length rule which mimics the body-min-length
    rule in its function. The default min length is set to 5 as per the
    discussion in Add title-min-length rule #138.

  • Add a few tests modeled on TitleMaxLength.

Signed-off-by: mr.Shu mr@shu.io

@mrshu mrshu force-pushed the mrshu/add-title-min-length branch 5 times, most recently from 4dd7315 to bbf88a5 Compare August 8, 2020 16:09
* Add a new `title-min-length` rule which mimics the `body-min-length`
  rule in its function. The default min length is set to 5 as per the
  discussion in jorisroovers#138.

* Add a few tests modeled on TitleMaxLength.

Signed-off-by: mr.Shu <mr@shu.io>
@mrshu mrshu force-pushed the mrshu/add-title-min-length branch from bbf88a5 to 83ad30f Compare August 8, 2020 16:12
@mrshu
Copy link
Contributor Author

mrshu commented Aug 8, 2020

@jorisroovers thanks a ton for a very nice tool!

I took a quick try on what could the implementation of #138 look like. Please feel free to let me know if there are things that would be worth updating -- should this end up really being part of gitlint, it would be nice to mention it in docs/rules.md as well for instance.

Thanks again!

cc @tjanez

Copy link
Owner

@jorisroovers jorisroovers left a comment

Choose a reason for hiding this comment

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

Left some comments/suggestions inline.

Some other places that need updating:

violation = rule.validate(u"å" * 72, None)
self.assertIsNone(violation)

# assert error on line length > 72
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
# assert error on line length > 72
# assert error on line length < 5

gitlint/rules.py Outdated
def validate(self, title, _commit):
min_length = self.options['min-length'].value
actual_length = len(title)
if 0 < actual_length < min_length:
Copy link
Owner

Choose a reason for hiding this comment

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

This would still allow for empty commit titles. I'd expect we'd want to capture this too?

Suggested change
if 0 < actual_length < min_length:
if actual_length < min_length:

I think it would be good to add a test for an empty title specifically to cover this scenario

@jorisroovers
Copy link
Owner

This is awesome, thanks so much for taking the time!

Left some comments in review.

I'm currently in a bit of a gitlint hiatus - which will affect when this is released. I will do it before the end of 2020, but might be a while... Appreciate your understanding!

* Update tests related to the `title-min-length` (mostly cosmetic
  changes)

* Add one more test for empty title.

* Add the new `title-min-length` rule to various documentation files.

Signed-off-by: mr.Shu <mr@shu.io>
@mrshu mrshu force-pushed the mrshu/add-title-min-length branch from 6907bf5 to 458d803 Compare August 20, 2020 07:52
@mrshu
Copy link
Contributor Author

mrshu commented Aug 20, 2020

Thanks for your response @jorisroovers !

I believe I've applied all the changes you've suggested with two notable exceptions:

  1. In the docs I was not sure what gitlint version to mention. This can be easily fixed once it comes to the release.
  2. I did not include TitleMinLength among the default rules for now, as there were quite a few fixtures that would need to get updated to take it into the account. If you think it's still worth doing, I'll update the fixtures as well.

I'm currently in a bit of a gitlint hiatus - which will affect when this is released. I will do it before the end of 2020, but might be a while... Appreciate your understanding!

Feel free to take as much time as would suite you -- I really do not think there is any rush here :)

Thanks again!

@jorisroovers jorisroovers added this to the 0.14.0 milestone Oct 6, 2020
@jorisroovers jorisroovers changed the base branch from master to main October 16, 2020 15:55
@jorisroovers
Copy link
Owner

Rebased this to the new main default branch. Will be fixing the conflicts and adding the missing code soonish (this weekend hopefully).

@mrshu
Copy link
Contributor Author

mrshu commented Oct 16, 2020

Thanks @jorisroovers -- please let me know if I can be of any help in here.

@jorisroovers jorisroovers changed the title feature: Add title-min-length New Rule: title-min-length Oct 18, 2020
@jorisroovers jorisroovers merged commit d542b14 into jorisroovers:main Oct 18, 2020
@jorisroovers
Copy link
Owner

Merged! Will go out with 0.14.0, which should be soon!

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

Successfully merging this pull request may close these issues.

None yet

2 participants