Skip to content
This repository has been archived by the owner on Feb 5, 2018. It is now read-only.

fix(cli): Fix cli to work with COMMIT_EDITMSG, custom path or text #92

Merged

Conversation

maxcnunes
Copy link
Contributor

Sometime ago I had included support to validate a commit message from a text instead of only from files.
The validation from a commit message argument stopped working on adding support for GIT GUI tool: maxcnunes/validate-commit-msg@d850cf4

This fixing makes CLI support 3 usage ways:

  1. Default usage is not passing any argument. It will automatically read from COMMIT_EDITMSG file.
  2. Passing a file path argument. For instance GIT GUI stores commit msg @GITGUI_EDITMSG file.
  3. Passing commit message as argument. Useful for testing quickly a commit message from CLI.

@codecov-io
Copy link

codecov-io commented Jun 22, 2017

Codecov Report

Merging #92 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #92   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines         126    126           
=====================================
  Hits          126    126

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b12933d...9c1fb7a. Read the comment docs.

@maxcnunes maxcnunes changed the title Fix cli fix(cli): Fix cli to work with COMMIT_EDITMSG, custom path or text Jun 22, 2017
@maxcnunes
Copy link
Contributor Author

@kentcdodds or @spirosikmd could you please review this? Thanks!

@Garbee
Copy link
Collaborator

Garbee commented Jun 24, 2017

Kent is no longer maintaining the project.

Is this keeping the original CLI behavior? If not we need to revert the original commit causing the breakage and add a unit test to cover it. Then retry a fix on the original problem.

@maxcnunes
Copy link
Contributor Author

@Garbee It is covering all possible behaviors for the CLI as I had described previously with the 3 usage ways. I will add tests for the CLI so we don't end up having another regression as that commit did in this case.

@Garbee
Copy link
Collaborator

Garbee commented Jun 24, 2017

It is covering all possible behaviors for the CLI

But is this in any way modifying the CLI from the way it worked before?

@maxcnunes
Copy link
Contributor Author

No. Still have the same behaviors. Initially the cli only worked validating the commit message from COMMIT_EDITMSG. In March I added support to validate also from an argument (my PR). And few days ago were added support to validate from git gui as well (commit). But that last change end up removing the support I had added previously.
This PR include back support for commit message as argument. Indepedently of which source is been used all them have the same validation behaviors.

@stevemao
Copy link
Contributor

stevemao commented Jun 25, 2017

does this fix the same problem as #87?

@maxcnunes
Copy link
Contributor Author

maxcnunes commented Jun 25, 2017 via email

Sometime ago I had included support to validate a commit message from a text instead of only from files.
The validation from a commit message argument stopped working on adding support for GIT GUI tool: d850cf4

This fixing makes CLI support 3 usage ways:
1. Default usage is not passing any argument. It will automatically read from COMMIT_EDITMSG file.
2. Passing a file path argument. For instance GIT GUI stores commit msg @GITGUI_EDITMSG file.
3. Passing commit message as argument. Useful for testing quickly a commit message from CLI.
@maxcnunes
Copy link
Contributor Author

@Garbee Any plans merging it to master? Or is there any reason for not accepting it?

@Garbee
Copy link
Collaborator

Garbee commented Jul 5, 2017

I just need to sit down and test it manually. Will do that tomorrow, was working on other issues that were filed earlier today when I went to work on the project.

@Garbee
Copy link
Collaborator

Garbee commented Jul 7, 2017

Just tested, it looks good to me. Merging now will sit down and chunk the release out by Monday if I can find the time this weekend.

Thank you!

@Garbee Garbee merged commit 8486495 into conventional-changelog-archived-repos:master Jul 7, 2017
@maxcnunes maxcnunes deleted the fix-cli branch July 7, 2017 15:32
Garbee added a commit that referenced this pull request Jul 20, 2017
There was an issue with a minor release, so this manual-releases.md
change is to release a new minor version.

Reference: #92
nickbalestra pushed a commit to nickbalestra/validate-commit-msg that referenced this pull request Jul 23, 2017
Addresses a bug introduces with the PR conventional-changelog-archived-repos#92 where the CLI wasn't exiting the process anymore in case
of error.
@nickbalestra nickbalestra mentioned this pull request Jul 23, 2017
nickbalestra pushed a commit to nickbalestra/validate-commit-msg that referenced this pull request Jul 23, 2017
Addresses a bug introduces with the PR conventional-changelog-archived-repos#92 where the CLI wasn't exiting the process anymore in case
of error.
nickbalestra pushed a commit to nickbalestra/validate-commit-msg that referenced this pull request Jul 23, 2017
Addresses a bug introduces with the PR conventional-changelog-archived-repos#92 where the CLI wasn't exiting the process anymore in case of error
Garbee pushed a commit that referenced this pull request Jul 23, 2017
Addresses a bug introduces with the PR #92 where the CLI wasn't exiting the process anymore in case of error
@krishna63
Copy link
Contributor

@maxcnunes : In my project, I am passing GITGUI_EDITMSG as some developers commit thru CLI and others thru GIT GUI. To achieve this we have made changes and created a PR and it was working fine in CLI and GIT GUI.
Now, I have updated to latest version, but i am unable to commit thru CLI, when i am passing the GITGUI_EDITMSG as a parameter to validate-commit-msg . With this PR code changes it works only in GIT GUI but not in CLI.

Scripts section of Package.json file
`
"scripts": {
"commitmsg": "validate-commit-msg GITGUI_EDITMSG",
}

`

cc: @Garbee , @kentcdodds

@Garbee
Copy link
Collaborator

Garbee commented Aug 3, 2017

@maxcnunes Do you think you'd be able to find the time to take a peek at why this change broke @krishna63's setup?

@krishna63 Do you think there is any way you could write a failing unit test for us to work against?

@maxcnunes
Copy link
Contributor Author

@krishna63 The problem is that you are specifying in your script to always use the GITGUI_EDITMSG file. Independently if you are committing from CLI or GUI tool. Since that file is only generated by GUI commits that script won't work with CLI commits.
We could add a fallback if the specified file doesn't exists then use the default one COMMIT_EDITMSG. But that could be misleading since you are specifying a file and the validator would end up using another. In that case I would prefer add support passing arguments to the this cli such as:

--gui (which would read from `GITGUI_EDITMSG`)
--file (which would read from a path)
--message (which would read from the input text)
# these 3 above couldn't be used together
--enable-fallback (which would read from `COMMIT_EDITMSG` in case the other rules failed)
# and if no argument provided would read from `COMMIT_EDITMSG` by default

For me that is the best approach. Is much more explicit of what is going on.

@Garbee I can work implementing the fallback by default (first option) or adding support for arguments (second option). Just let me know what you prefer.

@cmalard
Copy link
Contributor

cmalard commented Aug 3, 2017

Another approach : pass no argument and let the tool check if there is a GITGUI_EDITMSG or a COMMIT_EDITMSG :-)
This should cover both cases, allow us to remove the file option (it would become useless ?) and most important, the argument would be the same so everyone could work with the tool he wants without changing the config hook.

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

Successfully merging this pull request may close these issues.

None yet

6 participants