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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make conventional commits more explicit #292

Merged
merged 1 commit into from Oct 21, 2022

Conversation

Smankusors
Copy link
Member

Proposed changes

I updated the PR template and the contributing doc, so that it's easier for the first time contributors and us reviewers when handling the conventional commits format. Let me know what do you guys think 馃槈

(also merge two similiar checklist as one about change in docs)

Screenshots (if appropriate) or codepen:

n/a

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to change).

Checklist:

  • I have read the CONTRIBUTING document.
  • My commit messages follows the conventional commit format
  • My change requires a change to the documentation, and updated it accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

also merge two similiar checklist as one about change in docs
@Smankusors Smankusors added the meta Issues with the project itself or our GitHub repository. label Oct 18, 2022
@Smankusors Smankusors requested a review from a team October 18, 2022 17:15
Copy link

@DyegoCosta DyegoCosta left a comment

Choose a reason for hiding this comment

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

What about add a git commit hook instead? https://github.com/conventional-changelog/commitlint/#add-hook
Seems reasonably fast on my machine.

Screen Shot 2022-10-18 at 1 52 47 PM

@DanielRuf
Copy link

DanielRuf commented Oct 18, 2022

@DyegoCosta that's what husky does, setting up Git hooks.

In our setup it runs commitlint when a new commit is created.

https://github.com/materializecss/materialize/blob/main/.husky/commit-msg

https://github.com/materializecss/materialize/blob/main/package.json#L60

I think we should upgrade husky to the latest available version. Afaik it also needs a prepare script in package.json.

@wuda-io wants to check why it does not work. I guess this is due to the newer Git version which is not supported by husky or we forgot the prepare script. An upgrade of husky might be the easiest solution.

@DyegoCosta
Copy link

OH I did not notice we had that already in the project, I think my local was behind the upstream. Wouldn't be better to fix it instead of adding this new checkbox then? I added it to my outdated branch (with this command) and it worked as expected (screenshot above).

@wuda-io
Copy link
Member

wuda-io commented Oct 18, 2022

I think it only works on Unix machines, I will test it.
But since it is checked locally, it can bypassed anyhow, so an additional checkbox is not bad as reminder.

Copy link

@LoganTann LoganTann left a comment

Choose a reason for hiding this comment

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

Proposals approved

@Smankusors
Copy link
Member Author

@DyegoCosta sadly the commit hook doesn't work when the contributor edited directly from github.com or github.dev 馃槥

@DanielRuf
Copy link

@Smankusors that is as expected since the browser-based IDE has no real shell environment.

That should not be a problem for us. As we plan to onboard more members with write access, there will be more volunteers who can fix commit messages in PRs if needed.

@Smankusors Smankusors merged commit 43caedc into materializecss:main Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues with the project itself or our GitHub repository.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants