Add support for linting a range of commits natively. #21
Conversation
|
This bought the time to lint Zulip/Zulip down to 6 mins 43 sec on my 5400rpm harddisk.
|
gitlint/cli.py
Outdated
| for index, commit in enumerate(gitcontext.commits): | ||
| violations = linter.lint(commit) | ||
| if violations: | ||
| click.echo(u"{0}Commit {1}:".format( |
There was a problem hiding this comment.
I think we want to add an extra check here where we only print "Commit x" in case more than 1 commit has been linted. This ensures backwards compatibility as well.
gitlint/git.py
Outdated
| @@ -54,10 +54,11 @@ class GitCommit(object): | |||
| In the context of gitlint, only the git context and commit message are required. | |||
| """ | |||
|
|
|||
| def __init__(self, context, message, date=None, author_name=None, author_email=None, parents=None, | |||
| def __init__(self, context, message, sha, date=None, author_name=None, author_email=None, parents=None, | |||
There was a problem hiding this comment.
I think we should make sha an optional argument because we won't always have the sha available. For example, gitlint supports just piping arbitrary text into it for linting, in this case we don't have the sha (that's the reason why all these other parameters are also optional)
gitlint/git.py
Outdated
|
|
||
| # changed files in last commit | ||
| changed_files_str = sh.git("diff-tree", "--no-commit-id", "--name-only", "-r", "HEAD", **sh_special_args) | ||
| for sha in sha_list: |
There was a problem hiding this comment.
I like this a lot. Don't know what I hadn't thought about using a single call to git with a single --pretty line in the past.
53062ec to
8fe082e
Compare
|
I fixed some of the tests but this is mysterious (left and right are the same): The remaining failing tests are the integration tests where it have problems accessing the side effect of How should I go about fixing this? |
0ec6420 to
b0e9c4d
Compare
gitlint/tests/test_git.py
Outdated
| call('diff-tree', '--no-commit-id', '--name-only', '-r', sample_sha, **expected_sh_special_args) | ||
| ] | ||
| print(sh.git.mock_calls[1]) | ||
| print(expected_calls[1]) |
There was a problem hiding this comment.
Please remove these 2 print statements :)
gitlint/tests/test_git.py
Outdated
|
|
||
| expected_msg = u"An error occurred while executing 'git log -1 --pretty=%B': {0}".format(err) | ||
| expected_msg = u"An error occurred while executing 'git rev-list HEAD^...': {0}".format(err) |
There was a problem hiding this comment.
You need to escape ^ in expected_msg because the check below is a regex check, not an exact match check. So expected_msg should be:
expected_msg = u"An error occurred while executing 'git rev-list HEAD\^...': {0}".format(err)There was a problem hiding this comment.
Awesome work :)
Re: the assertion error, see my inline comment, that has to do with some regex escaping.
Re: integration tests, I figured out that git ref-list HEAD^... doesn't work for repositories with only a single commit in them. The integration tests fail because of this as they create a new repo with a single commit per test.
Even though it's an edge case, the 'git repo with a single commit' scenario will obviously need to be supported. I believe a potential workaround is using git rev-list --max-count 1 HEAD in case the user has not specified a custom refspec. To make that happen, we probably need to change the default value of commits to None and use git rev-list --max-count 1 HEAD if it's None and git rev-list <custom-refspec> otherwise. There might be other ways to do this too, but my quick research hasn't found how to do it with just changing the default refspec value from HEAD^... to something else.
On a different note, we probably also want to add some unit and or integration tests to specifically test this new scenario of linting multiple commits. I can help out with that too after merging this PR if you prefer that.
b0e9c4d to
5f89670
Compare
4249afc to
7d15ee0
Compare
This allows user to specify a range of commits using the --commits cli option. It currently accepts either an individual commit hash (eg: gitlint --commits 6f29bf8) or a range of commits using the git range syntax (eg: gitlint --commits 9c6f70d...HEAD). Fixes jorisroovers#14.
7d15ee0 to
3a0c001
Compare
|
Thanks for the help! I fixed all the tests and also took advantage of the I will try to add new tests and docs after the merge of this PR. BTW are you going to do a small release for this feature so Zulip don't have to wait till the release of v0.9.0? |
|
Thank you very much for this excellent contribution! We now just have to get the docs updated and some new unit and integration tests added for this new use-case. If you work on the testing, I can add the docs. There are a few other minor changes I'm making to gitlint, I'm planning to release 0.8.1 in the next week or so. Thanks again for this, very much appreciated! |
Fixes #14.