-
Notifications
You must be signed in to change notification settings - Fork 73
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
Added commit message style check #23
Conversation
1 similar comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this feature which was added by @atb00ker :
- https://github.com/openwisp/openwisp-utils/blob/master/setup.py#L36
- https://github.com/openwisp/openwisp-utils/blob/master/openwisp_utils/qa.py
We want to create something similar which we can reuse in other repositories too. That's the goal.
If something is not clear ask questions here.
ok i'll try to move my code. |
You have errors with commit message:
|
@nemesisdesign @ppabcd I feel this is a bit complicated way to solve this problem. What do you guys think? |
maybe i'll try debug |
@atb00ker sounds good. Let's keep in mind we'll have to implement this check in We want this check to run only on pull-requests, because once a commit is done on master, with the new branch protection rules in place we can't change it (that means admins will have to be forced to use more and more development branches and avoid working on master, which is a good thing). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ppabcd in case you are wondering what means "mentions an issue without closing it", the example I had given was the following: Implements #20
; this example won't close the issue #20
, because only the keywords fix
, fixes
, close
and closes
do it.
Another example is: Closes #20 and #21
; this won't close #21
because #21
is not preceded with the keyword which closes the issue.
But we want to allow writing something like Related to #33
, because sometimes we
may want to just reference an issue even though we are not solving it in that commit.
That means that your algorithm should look for patterns which contain a hashtag preceded by a space and followed by a number and a space, eg: #<number>
, and if that pattern is not preceded with any of the following keywords (case insensitive check) it should raise an error: fix
, fixes
, close
, closes
, rel
, related to
, ref
, refers to
.
Please also make sure you collect all the errors in a list and only stop execution once all the checks have been done, that way when you stop execution you can list all the errors that have been found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ppabcd overall you're doing great Reza, keep improving it with our feedback and we'll achieve this goal as well and our test suites will become very robust and we won't need to spend so much time with trivial reviews anymore, that's really good! 👍
setup.py
Outdated
@@ -33,7 +33,8 @@ | |||
packages=find_packages(exclude=['tests', 'docs']), | |||
entry_points={ | |||
'console_scripts': [ | |||
'checkmigrations = openwisp_utils.qa:check_migration_name' | |||
'checkmigrations = openwisp_utils.qa:check_migration_name', | |||
'commitcheck = openwisp_utils.qa:check_commit_message', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use checkcommit
for consistency
tests/test_project/tests/test_qa.py
Outdated
|
||
MIGRATIONS_DIR = path.join(path.dirname(path.dirname(path.abspath(__file__))), 'migrations') | ||
|
||
|
||
class TestQa(TestCase): | ||
_test_migration_file = '%s/0002_auto_20181001_0421.py' % MIGRATIONS_DIR | ||
_gh_token = os.environ["GH_TOKEN"] | ||
_repo = os.environ["TRAVIS_REPO_SLUG"] | ||
_commit = os.environ["TRAVIS_COMMIT"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to do this here, you need to supply your own test values directly
tests/test_project/tests/test_qa.py
Outdated
def test_qa_call_check_commit_message_pass(self): | ||
options = [ | ||
'commitcheck', '--token', self._gh_token, | ||
'--commit', self._commit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's ok to hardcode values here, you should test all the pass cases that we mentioned in the discussion
tests/test_project/tests/test_qa.py
Outdated
options = [ | ||
[ | ||
'commitcheck', '--token', self._gh_token, | ||
'--commit', self._commit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's ok to hardcode values here, you should test all the failure cases that we mentioned in the discussion
openwisp_utils/qa.py
Outdated
|
||
if not args.token: | ||
if "GH_TOKEN" in os.environ: | ||
github_token = os.environ["GH_TOKEN"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how to coverage this line in travis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels wrong to have this, why you can't just pass that token as argument when calling the script instead?
This check shouldn't know anything about tokens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ppabcd Feels a lot more complex than what's really needed.
I think the check should take a string that represents the commit message and check just that.
In travis we can find a way to get the commit message and pass it somehow to the check.
Focus on the logic which checks the commit message right now.
Calling the github API is not really needed.
openwisp_utils/qa.py
Outdated
|
||
def check_commit_message(): | ||
args = _commit_check_args() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove blank line (increase line-height of your editor if you feel the need of doing this)
openwisp_utils/qa.py
Outdated
|
||
if not args.token: | ||
if "GH_TOKEN" in os.environ: | ||
github_token = os.environ["GH_TOKEN"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels wrong to have this, why you can't just pass that token as argument when calling the script instead?
This check shouldn't know anything about tokens
tests/test_project/tests/test_qa.py
Outdated
@@ -44,5 +44,70 @@ def test_qa_call_check_migration_name_failure(self): | |||
else: | |||
self.fail('SystemExit or Exception not raised') | |||
|
|||
def test_qa_call_check_commit_message_pass(self): | |||
options = [ | |||
'commitcheck', '--token', os.environ["GH_TOKEN"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should fake the token during tests
tests/test_project/tests/test_qa.py
Outdated
def test_qa_call_check_commit_message_pass(self): | ||
options = [ | ||
'commitcheck', '--token', os.environ["GH_TOKEN"], | ||
'--commit', os.environ["TRAVIS_COMMIT"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is --commit
the commit message? Or the commit hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit hash
tests/test_project/tests/test_qa.py
Outdated
options = [ | ||
'commitcheck', '--token', os.environ["GH_TOKEN"], | ||
'--commit', os.environ["TRAVIS_COMMIT"], | ||
'--repo', os.environ["TRAVIS_REPO_SLUG"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you apply my suggestion of using git log -1
you don't need this
You have errors with commit message:
|
1 similar comment
You have errors with commit message:
|
All check done, no errors. |
I get more problem :
is you mean first line and second line in description?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress.
Please address my comments, clean up the code, fix the travis build, squash the commits and update the PR.
We will help you to finish the PR with more suggestion but the code is too messy now.
.travis.yml
Outdated
- "3.5" | ||
- "2.7" | ||
- '3.5' | ||
- '2.7' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't chenge these lines please
.travis.yml
Outdated
@@ -33,12 +36,12 @@ install: | |||
- pip install -e .[users] .[qa] | |||
|
|||
before_script: | |||
- ./runflake8 | |||
- ./runisort | |||
- "./runflake8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't change these lines please
.travis.yml
Outdated
|
||
branches: | ||
only: | ||
- master | ||
- master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't change these lines please
.travis.yml
Outdated
- python: '2.7' | ||
env: DJANGO="django>=2.0,<2.1" | ||
- python: '2.7' | ||
env: DJANGO="django>=2.1,<2.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't change these lines please
.travis.yml
Outdated
|
||
env: | ||
# Secure ENV Vars | ||
secure: d75/sJr04cAnKgJ9JJvZZVWwRzxczNE5UfMXnluaYllraApX03NZugrv4RBp60RtUm/Sa1uv6uZiv1Xvw0VArGvNk/HvL9/FTw7riqA/YulsxNd2Hy3xyvmn4tAkbQGI0VIRh1TmFDx3DQLw/DZJ72DZDee3YWqzSO+W+z9Zs9QP6UjiiIepc6fi1+pZ6jeIn9iqg7PlVJG5QOBH0oLNA0scSWLlueyDhXR+7nteXFQ4IyA9EVPMzcUjv0kVsUCbsMDYJj3JstfuTq+mjoFbrzwX8Ey4mfLIaWpL4pXjL0i6ErZuT36l5Q0pGog/5+snKAJy02BVC9rmt0sX2IWBmWTa2fQhEZ20X9vO3/jYYbttmnYIxQVGWqMaco6e+ktxjBtL+00tMabv6z9taYGsR3b5UEWJYZGi3AENLrYuYrlp9SFUWMwRDWhaTm180DjIvdQDOzMwkmH+aEMnex3kqeF56dwyoIMhJIhg6I27PbsHkbkc9wNrPP9PN1Rke0PRsYbSx8QLQWGifCf30bJ/gZNwp41mdczgll6QNvdP6LcxE/aN0q3f/zD0XceP70G1klMOTAgzJZF6YxMkx64at+35wwaIpHBE+UpPfmI4uj2FMZgb5lWISWkJIR6Z9P5yAtSZ/CtFNTTRwohd0nflEO67od5+C18M3u9weoSXz5U= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is not used, remove it
openwisp_utils/qa.py
Outdated
# if not args.token: | ||
# raise Exception("CLI arguement `token` is required " | ||
# "but not found") | ||
if not args.repo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need this argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this argument will be deleted
tests/test_project/tests/test_qa.py
Outdated
# '--token', | ||
# os.environ["GH_TOKEN"], | ||
'--repo', os.environ["TRAVIS_REPO_SLUG"], | ||
'--message', os.environ['TRAVIS_COMMIT_MESSAGE'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please stop using environment variables, remove all of them and use hardcoded test values, you are testing your implementation here, not doing the actual check with the live commit. The test needs to be stable and replicable.
Write some sample commit messages that will pass the check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i'll try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all good commit messages, use these ones as starting point
[qa] Minor clean up operations
(it's ok to not reference any issue sometimes)
[qa] Improved Y #2
Related to #2
(but if an issue is mentioned it should be specified whether the commit fixes it or just references it)
[qa] Finished task #2
Closes #2
Related to #1
(multiple references can be added in the extended desc)
These commit messages must not fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get more problem :
In travis-ci.org i can't access commit title. You can see different here with my travis
I can't create encrypted GH_TOKEN and now i disable comment notification and only return travis errors.
Don't worry, remove all of this. You don't need to access the commit title from travis.
No blank line before first line and second line I see when you added title Test with description Testing Commit. Github will create commit messages like this.
TestingTesting Commit
is you mean first line and second line in description?
Testing
is the first line (aka short description)
Testing Commit
is the second line (aka extended commit description)
There must be a blank line between the two.
I am currently looking for an algorithm Mentions an issue without closing it. Because i don't have much experience in regex.
Try the the following:
- use a pattern that allows you to find out where
#<issue-number>
are located in the extended description (don't look in the short description because we allow to simply write#<issue-number>
there - when you find occurrences of the pattern, for each pattern do the following:
- find out at which length of the text they occur
- use the length you just found to slice the text from the beginning to the end of the pattern you have found
- split the words using spaces,
parts = text.split(' ')
- the last element,
part[-1]
should be the issue reference (#<issue-number>
), whilepart[-2]
should be the word that precedes it - check if the preceding word is allowed, eg:
allowed_words = ['fix',
'fixes',
'close',
'closes',
'related to',
'refers to',
'ref',
'rel']
word = part[-2]
if word.lower() not in allowed_words:
# fail here..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more comments
openwisp_utils/qa.py
Outdated
api_title = commit['title'] | ||
|
||
if commit['message'] != '': | ||
api_message = commit['message'].split("\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename all variables that have the api_
prefix so that they don't have it anymore
openwisp_utils/qa.py
Outdated
) | ||
else: | ||
# Check capital after prefix | ||
commitMessagefirst = api_title.replace(prefix.group(), '').strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not use camel case variables in python
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
address this comment
openwisp_utils/qa.py
Outdated
# Thank you https://gist.github.com/simonw/091b765a071d1558464371042db3b959 | ||
def _get_commit(): | ||
leading_4_spaces = re.compile('^ ') | ||
lines = _shell_exec('git log -1').split('\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to do this, we'll pass output of git log via the bash shell
openwisp_utils/qa.py
Outdated
# api_token = args.token | ||
|
||
if args.message: | ||
api_title = args.message.split("\n")[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is api_title
the commit short desc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change all these variable names so they don't mention api anymore, if this is the short desc, name it short_desc
, apply the same suggestion to all variables
tests/test_project/tests/test_qa.py
Outdated
# '--token', | ||
# os.environ["GH_TOKEN"], | ||
'--repo', os.environ["TRAVIS_REPO_SLUG"], | ||
'--message', os.environ['TRAVIS_COMMIT_MESSAGE'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all good commit messages, use these ones as starting point
[qa] Minor clean up operations
(it's ok to not reference any issue sometimes)
[qa] Improved Y #2
Related to #2
(but if an issue is mentioned it should be specified whether the commit fixes it or just references it)
[qa] Finished task #2
Closes #2
Related to #1
(multiple references can be added in the extended desc)
These commit messages must not fail.
tests/test_project/tests/test_qa.py
Outdated
'[qa] Hello World.\nTest\nFixes #20', | ||
'--repo', "ppabcd/openwisp-utils" | ||
], | ||
['commitcheck'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other common bad commit messages you should add to the failing tests:
[qa] Finished task X #2
Closes #2
no blank line between short desc and extended desc
b54872c
to
9669de8
Compare
openwisp_utils/qa.py
Outdated
"created have a descriptive name. If " | ||
"default name pattern is found, " | ||
"raise exception!") | ||
parser.add_argument("--token", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't you remove this?
openwisp_utils/qa.py
Outdated
help="Github token for check commit") | ||
parser.add_argument("--message", | ||
help="Commit message") | ||
parser.add_argument("--repo", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't you remove this?
openwisp_utils/qa.py
Outdated
# api_token = args.token | ||
|
||
if args.message: | ||
api_title = args.message.split("\n")[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change all these variable names so they don't mention api anymore, if this is the short desc, name it short_desc
, apply the same suggestion to all variables
openwisp_utils/qa.py
Outdated
api_title = args.message.split("\n")[0] | ||
if len(args.message.split("\n")) > 1: | ||
api_message = args.message.split("\n")[1:] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove blank line
openwisp_utils/qa.py
Outdated
# Check dot in end of the first line | ||
if api_title[len(api_title.strip()) - 1].strip() == '.': | ||
errors.append("Do not add a final dot at the end of the first line.") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove blank line
openwisp_utils/qa.py
Outdated
commitMessagefirst = api_title.replace(prefix.group(), '').strip() | ||
if not commitMessagefirst[0].isupper(): | ||
errors.append("No capital letter after prefix") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove blank line
openwisp_utils/qa.py
Outdated
hastag = re.search(r'\#(\d+)', api_title) | ||
if not hastag: | ||
errors.append("No mention issues in the short description") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove blank line
openwisp_utils/qa.py
Outdated
if args.message.split("\n")[1] != '': | ||
errors.append("No blank line before first " | ||
"line and second line") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove blank line
openwisp_utils/qa.py
Outdated
word = parts[num - 2] | ||
if word.lower() not in allowed_words: | ||
errors.append("Mentions an issue without closing it") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove blank line
tests/test_project/tests/test_qa.py
Outdated
'commitcheck', | ||
'--message', | ||
"[qa] Updated more file and fix " | ||
"problem #20\n\nAdded more files Fixes #20" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all good commit messages, use these ones as starting point
[qa] Minor clean up operations
(it's ok to not reference any issue sometimes)
[qa] Improved Y #2
Related to #2
(but if an issue is mentioned it should be specified whether the commit fixes it or just references it)
[qa] Finished task #2
Closes #2
Related to #1
(multiple references can be added in the extended desc)
These commit messages must not fail, add these to this test case please.
927355d
to
8155e56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ppabcd is this ready? Or is it still pending changes?
ready @nemesisdesign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the comment below. 😄
fc9ea11
to
51e68e8
Compare
i have done updated description messages @atb00ker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm asking these last round of changes and then we are done
openwisp_utils/qa.py
Outdated
if short_desc_location: | ||
# Get all issues in long description | ||
long_desc_issues = _check_word(message) | ||
if(not _is_array(long_desc_issues)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rewrite this as if not _is_array(long_desc_issues):
openwisp_utils/qa.py
Outdated
body = "You have errors with commit message: \n" | ||
for e in errors: | ||
body += "- " + e + "\n" | ||
if(len(errors) > 0): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if len(errors) > 0:
all done @nemesisdesign @atb00ker |
Google Code-in Task : https://codein.withgoogle.com/dashboard/task-instances/5188396768034816/
Fixes #20