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

cEP-0016.md: Commit Content Inspection #114

Merged
merged 1 commit into from
Jul 28, 2018
Merged

Conversation

kriti21
Copy link
Contributor

@kriti21 kriti21 commented May 3, 2018

This cEP the coala process rules which
will be enforced by GitCommitBear to handle
special types of commit messages like
git revert and git merge.

cEP-0016.md Outdated
encoding = diff.headers.getparam('charset')

patch1 = PatchSet(diff1, encoding=encoding)
patch2 = PatchSet(diff1, encoding=encoding)
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean diff2 ?

cEP-0016.md Outdated
patch2 = PatchSet(diff1, encoding=encoding)

# compare patch1 and patch2 using unidiff
if not(patch1) == patch2:
Copy link
Member

@refeed refeed May 5, 2018

Choose a reason for hiding this comment

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

I think practically probably we wanna do if not patch1 == patch2: (without the parentheses). However, this is just a draft, so it doesn't matter here ;)

cEP-0016.md Outdated
@@ -0,0 +1,178 @@
Commit Content Inspection
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be "Git Commit Content Inspection" ?

cEP-0016.md Outdated
| Status | Implementation due |
| Type | Process |

# Abstract
Copy link
Member

@refeed refeed May 6, 2018

Choose a reason for hiding this comment

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

I think we can change this "Header 1" title to a "Heading 2" title (## Abstract)
and also change the "Header 1" which are below this line

"Header 1" is usually just used once and it just used for the title of the document itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@kriti21 kriti21 force-pushed the cEPgsoc18 branch 2 times, most recently from bc9703c to 20d86e5 Compare May 6, 2018 04:22
cEP-0016.md Outdated
This document describes coala process rules which will be enforced by the
GitCommitBear.

This cEP describes some new changes in the implementation of GitCommitBear
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have "this document describes" twice? It sounds weird while reading it. Can't you just write it in one paragraph?

Like "This document describes coala process rules which will be enforced by the GitCommitBear and some new changes in its implementation.........."

cEP-0016.md Outdated

The GitCommitBear at present performs a check on the content of regular commits
made to coala. However, there are many special types of commit messages that
should be only used in conjunction with patches containing a special type of
Copy link
Member

Choose a reason for hiding this comment

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

should be only used -> should only be used

Copy link

@seblat seblat left a comment

Choose a reason for hiding this comment

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

Looks fine to me regarding the comments which have already been made

cEP-0016.md Outdated
Here is the detailed implementation stepwise:

1. We start by identifying different types of special commits namely revert,
merge and travis commits.
Copy link
Member

Choose a reason for hiding this comment

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

indent.

also please run a markdown validator on your file.

Copy link
Member

Choose a reason for hiding this comment

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

#128 will enforce this.

Copy link
Member

@damngamerz damngamerz left a comment

Choose a reason for hiding this comment

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

LGTM :)
Well done on the mockups.

cEP-0016.md Outdated
2. Each type of commit is then handled as a separate case.
3. For travis commits:
* Merged commits are identified first.
* It's checked whether the merge commit contains `[ci skip]`.
Copy link
Member

Choose a reason for hiding this comment

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

no. [ci skip] is not a 'travis commit'

Please read more about [ci skip]

Copy link
Member

@refeed refeed left a comment

Choose a reason for hiding this comment

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

lgtm

@kriti21 kriti21 force-pushed the cEPgsoc18 branch 3 times, most recently from a83ea49 to bb414fe Compare July 11, 2018 23:22
cEP-0016.md Outdated
it is not a clean revert is expected.
- A message suggesting improvements in the commit message is displayed.

## Code Prototypes
Copy link
Member

Choose a reason for hiding this comment

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

I think we might want to get rid of this section since it changes everytime the PR for VCSCommitBear updated, and it blocks this cEP to get merged faster. The explanation above is good enough imo :)

cEP-0016.md Outdated
corresponding CI engine allows skipping build or not. An appropriate message
is returned if skipping build is not supported.
- If the commit type from the output of `VCSCommitBear` indicates that the
commit skips CI build, inspection is done inside `CISkipInspectBear`
Copy link
Member

Choose a reason for hiding this comment

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

Also update this, VCSCommitBear is no longer providing COMMIT_TYPE.ci_skip_commit, all of the matching is done inside of the CISkipInspectBear.

cEP-0016.md Outdated
perform a `git rebase` instead.
7. For revert commits:
- If the commit type from the output of `VCSCommitBear` indicates that the
commit is a `git revert` commit, inspection is done in
Copy link
Member

Choose a reason for hiding this comment

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

Update this also

cEP-0016.md Outdated
- If the commit type from the output of `VCSCommitBear` indicates that the
commit is a `git merge` commit, inspection is done in `GitMergeInspectBear`
otherwise return.
- `GitMergeInspectBear` will be used by find if the commit tries to merge
Copy link
Member

Choose a reason for hiding this comment

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

by find

by finding

cEP-0016.md Outdated
- `GitCommitBear` would identify and check contents of the unmerged parent of
these merge commits.
4. A meta bear called `VCSCommitBear` would be created that would return a
`HiddenResult` comprising of a tuple that will contain information about
Copy link
Member

Choose a reason for hiding this comment

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

HiddenResult

CommitResult

@jayvdb
Copy link
Member

jayvdb commented Jul 28, 2018

ack 386ddb3

@jayvdb
Copy link
Member

jayvdb commented Jul 28, 2018

@gitmate-bot rebase

@gitmate-bot
Copy link

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

This cEP describes the coala process rules
which will be enforced to handle special types
of commit messages like git revert and git merge.

Closes coala#112
@gitmate-bot
Copy link

Automated rebase with GitMate.io was successful! 🎉

@jayvdb
Copy link
Member

jayvdb commented Jul 28, 2018

@gitmate-bot ff

@gitmate-bot
Copy link

Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently ⚠️

@gitmate-bot gitmate-bot merged commit ea25f09 into coala:master Jul 28, 2018
@gitmate-bot
Copy link

Automated fastforward with GitMate.io was successful! 🎉

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

Successfully merging this pull request may close these issues.

10 participants