conventional-changelog-archived-repos / validate-commit-msg

DEPRECATED. Use https://github.com/marionebl/commitlint instead. githook to validate commit messages are up to standard
http://conventionalcommits.org/
MIT License
556 stars 101 forks source link

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

Closed maxcnunes closed 7 years ago

maxcnunes commented 7 years ago

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@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 commented 7 years ago

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 commented 7 years ago

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

Garbee commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

does this fix the same problem as https://github.com/conventional-changelog/validate-commit-msg/pull/87?

maxcnunes commented 7 years ago

Yes, It does. And is also covering a problem which I have commented in that PR.

On Sun, Jun 25, 2017, 4:26 AM Steve Mao notifications@github.com wrote:

Is it fixing the same problem as #87 https://github.com/conventional-changelog/validate-commit-msg/pull/87?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/conventional-changelog/validate-commit-msg/pull/92#issuecomment-310887573, or mute the thread https://github.com/notifications/unsubscribe-auth/AAphpNknU9S4Vuxv_5-wrqBCqW8LflNSks5sHguwgaJpZM4OCZlL .

maxcnunes commented 7 years ago

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

Garbee commented 7 years ago

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 commented 7 years ago

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!

krishna63 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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.