-
Notifications
You must be signed in to change notification settings - Fork 17
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
package: Add pre-commit hooks to enforce commit message style #12
Conversation
"feat", | ||
"deps", | ||
"fix", | ||
"docs", | ||
"package", | ||
"style", | ||
"refactor", | ||
"test", | ||
"revert", | ||
"WIP" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this set looks fine to me, same comment about recent commits failing this, but then again there wasn't a standard before 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is all mostly standard, so I just went with it. I added deps, package, WIP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see I used improvement
recently, but that's kinda like feat
. Technically, everything is an improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darinspivey, which one is gonna be used for deprecating the feature? For example, on LogDNA Agent
, I have recently removed WinEvent Logging Component
; so, which one from this list would be the best one describing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say refactor for that. Then the subject can still say something about "refactor: removed xx because blah blah"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work for backward-incompatible removals too? Just wondering...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it only applies moving forward with new commits. I don't think we need to worry about retroactively applying it.
We should do our best to enforce our desired conventional commit style, especially since PRs may come from the public. This sets up basic rules for validating the conventional commit style. Semver: minor Ref: LOG-7183
328770e
to
d6efdef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add this to everything written on js
, right? So, what's stopping us from applying the same thing onto (e.g.) the agent or you have already started doing that?
We will definitely add this across public repositories. I'm just using this as a way for us to feel it out. |
We should do our best to enforce our desired conventional
commit style, especially since PRs may come from the
public. This sets up basic rules for validating the
conventional commit style.
Semver: minor
Ref: LOG-7183