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

Troublesome commitlint #93

Closed
abdallahalsamman opened this issue Dec 7, 2018 · 14 comments
Closed

Troublesome commitlint #93

abdallahalsamman opened this issue Dec 7, 2018 · 14 comments
Labels
discussion Discussion of changes or bikeshedding released

Comments

@abdallahalsamman
Copy link
Member

Since we've added commitlint, I've discussed it being removed.

Here's what I tried to just do a very basic thing:

git ci -am "fix: Merge Develop; Add Storybook and Backstopjs tests;"
git ci -am "feat(testing): Merge Develop; Add Storybook and Backstopjs tests;"
git ci -am "feat(testing): Merge Develop and add Storybook and Backstopjs tests"
git ci -am "feat(Testing): Merge Develop; Add Storybook and Backstopjs tests;"
git ci -am "Feat(testing): Merge Develop; Add Storybook and Backstopjs tests;"
git ci -am "feat(testing): Merge Develop and add Storybook and Backstopjs tests" --no-verify

Every time once of these got me an error, I tried to modify my commit messages based on the error only to get other errors.

I was done, and I wanted to use --no-verify to just skip this linting for commit message, but still, I get an error.

husky > prepare-commit-msg hook failed (cannot be bypassed with --no-verify due to Git specs)

Do we REALLY need this? I see this as anti-productive instead of the other way around.

@abdallahalsamman
Copy link
Member Author

abdallahalsamman commented Dec 7, 2018

More on this, still trying to commit

  git ci -am "feat(testing): Merge Develop and add Storybook and Backstopjs tests" --no-verify
  git ci -am "feat(test): Merge Develop and add Storybook and Backstopjs tests" --no-verify
  git ci -am "feat(test): Merged Develop and add Storybook and Backstopjs tests" --no-verify
  git ci -am "feat(test): Merged Develop and added Storybook and Backstopjs tests" --no-verify
  git ci -am "feat(test) Merged Develop and added Storybook and Backstopjs tests" --no-verify

Errors I get are

✖   message may not be empty [subject-empty]
✖   type may not be empty [type-empty]
✖   subject must not be sentence-case, start-case, pascal-case, upper-case [subject-case]

What is the subject anyway?

My point is: this just doesn't feel natural, and its very annoying

@BcRikko
Copy link
Member

BcRikko commented Dec 7, 2018

How about making this loose rule? 🤔
2018-12-07 18 35 22

This rule is used in vuejs project.
https://github.com/vuejs/vue/blob/dev/scripts/verify-commit-msg.js

@trezy What do you think? 🤔

@guastallaigor
Copy link
Member

More on this, still trying to commit

  git ci -am "feat(testing): Merge Develop and add Storybook and Backstopjs tests" --no-verify
  git ci -am "feat(test): Merge Develop and add Storybook and Backstopjs tests" --no-verify
  git ci -am "feat(test): Merged Develop and add Storybook and Backstopjs tests" --no-verify
  git ci -am "feat(test): Merged Develop and added Storybook and Backstopjs tests" --no-verify
  git ci -am "feat(test) Merged Develop and added Storybook and Backstopjs tests" --no-verify

Errors I get are

✖   message may not be empty [subject-empty]
✖   type may not be empty [type-empty]
✖   subject must not be sentence-case, start-case, pascal-case, upper-case [subject-case]

What is the subject anyway?

My point is: this just doesn't feel natural, and its very annoying

I don't think you can use upper-case letters anywhere in the commit.
Have you tried this?

feat(test): merged develop and added storybook and backstopjs tests

@abdallahalsamman
Copy link
Member Author

@guastallaigor that turns out to be true, when I used lower-case letters it worked, but that's not how I expect it to work, and that's why I opened this issue 👍

@guastallaigor
Copy link
Member

@evexoio Yes I understand, I'm used to commit using upper-case letters aswell, but in this repo I'm trying to get used to commiting using only lower-case letters.

@abdallahalsamman
Copy link
Member Author

More to this, if you end your commit message with a dot it'll get rejected as well 🤧

@trezy
Copy link
Member

trezy commented Dec 7, 2018

All of the commitlint settings we're using are based on the Angular Commit Conventions. Please read through those. I've been using these conventions in a great many projects to date. There are several substantial benefits that come from them, but here are the biggest ones:

  1. Commits are easy to read
    Reviewing PRs is often one of the most time consuming parts of running an open source project. Forcing commits into a standard format makes reviewing PRs much faster, and it makes determining the value of a commit much easier.

  2. Changelogs can be generated automatically
    Check out this changelog example. By using properly formatted commit messages, we can generate changelogs automatically that are actually useful to us.

Now, I will say that the sentence-case rule on commit subjects regularly trips me up as well. I'm not opposed to the idea of switching that rule off so that commit titles can start with uppercase characters and end with a dot (.) if the rest of the team wants to do that. I'm also curious about @BcRikko's thoughts with all of these changes so far.

@abdallahalsamman
Copy link
Member Author

abdallahalsamman commented Dec 8, 2018

Commits are easy to read

Agreed, I can see that already. and we can keep that the way it is with a less strict rule to achieve balance between commit message's clarity and the ease of actually commiting.

@trezy can we apply the changes you and @BcRikko suggested please?

@abdallahalsamman abdallahalsamman added the waiting - contributor Waiting for the contributor to address some situations label Dec 8, 2018
@trezy
Copy link
Member

trezy commented Dec 8, 2018

@evexoio Absolutely! I’ll take a look at that tomorrow. I’m hosting a big holiday party today.

If anybody wants to look into it sooner, you’ll want to look into commit-lint rules to implement the changes to the subject format, and cz-customizable to alter the available commit types.

@trezy trezy added discussion Discussion of changes or bikeshedding and removed waiting - contributor Waiting for the contributor to address some situations labels Dec 9, 2018
trezy added a commit that referenced this issue Dec 9, 2018
Commit subjects can now start with capital letters and end with periods.

closes #93
@4k1k0
Copy link
Contributor

4k1k0 commented Dec 9, 2018

Please, could someone explain me how to make a commit from the command line?

This is what I'm doing:

  • Clone my fork
  • Change to develop branch
  • Make some changes
  • Create a new branch
  • Adding the modified files to the stage
  • Commit

I've trying to do it since this morning but I don't understand husky. According to the docs git commit -m 'Message' should do the job, but I keep getting this error message:

✖   message may not be empty [subject-empty]
✖   type may not be empty [type-empty]
✖   found 2 problems, 0 warnings

What I am doing wrong? 😟

🙇‍♂️ 🙏

@guastallaigor
Copy link
Member

guastallaigor commented Dec 9, 2018

Hello @4k1k0, as up to this moment all messages must be like this type: msg.
No dot at the end of the line.
Only lower-case letters.
Example: feat: add new feature or feat(icons): add new icon
You can also use npm run commit that can help you with that
But some things will be changed to make this rules more loose.

@4k1k0
Copy link
Contributor

4k1k0 commented Dec 9, 2018

Thank you so much! It worked like a charm.

@abdallahalsamman
Copy link
Member Author

@4k1k0 @guastallaigor actually I merged #144

Upper-case letters are ok now, as well as a dot at the end of a sentence :)

@BcRikko
Copy link
Member

BcRikko commented Dec 18, 2018

🎉 This issue has been resolved in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion of changes or bikeshedding released
Projects
None yet
Development

No branches or pull requests

5 participants