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
feat: added examples to the specification #21
Conversation
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 like to add some examples :)
It would be better to add more. EG: Referencing an issue (EG: Fixes #1)
Also I'm not sure if breaking change is a body (probably footer?). A body is a long description of what's being changed.
index.md
Outdated
BREAKING CHANGE: `extends` key in config file is now used for extending other config files | ||
``` | ||
|
||
### Commit message with scope |
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 this is the most commonly used message so I'd reorder this to top :)
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'll fix this.
index.md
Outdated
|
||
### Commit message with no body | ||
``` | ||
fix: Correct spelling of CHANGLOG |
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.
changing the CHANGELOG should be docs.
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.
Ironically, I got this example from this repo. #13 :-D
But I'll fix it.
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.
Well, it was wrong :)
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.
subject should not start with caps, this spells trouble 🗡
@tchaffee thank you for this work; just some quick drive by comments (I'm finally catching up on some of my massive backlog of git issues phew). I love the idea of adding some example; I'm excited to see this work to conclusion. a couple random notes:
@Morishiri perhaps you have some thoughts on this pull too; given that you've made some significant contributions to this site. |
I will update polish language as well with some additional changes which I've noticed after I added this translation, but I think I will do that as separate pull request? Or should I add it here? |
@Morishiri @tchaffee this has been hanging out for a long time, shall we get it over the finish line? |
015d7ae
to
d684c53
Compare
I think this should be good now, but if there are any other minor changes please let me know as soon as you can and I'll get this finished up. |
index.md
Outdated
|
||
### Commit message for a fix using an (optional) issue number. | ||
``` | ||
fix: issue #12 - minor typos in code |
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 issue #12
should be used in the footer and not in the commit message. See the doc for more info.
Other examples seem good to be 😄
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.
So that means I have to also include a body? That's kind of annoying if there is otherwise no need for a body. I'll change this for now, but I think this rule should be reconsidered.
|
||
### Commit message with scope | ||
``` | ||
feat(lang): added polish language |
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.
WDYT about using "imperative sentence" in the examples?
This is a shared best practice (also in the angular repo)
feat(lang): add polish language
PS: This is not enforced by this convention btw
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.
If it's not enforced by this convention, could you please open a new issue to discuss this?
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 second this. As I mentioned on my other comment, even if it's not in the spec we shouldn't have "bad" commit messages in the examples. Especially if people would end up relying on tools to enforce them that'd scream at this.
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.
We should have "bad" commit messages if the spec doesn't require "good" ones. By trying to include best practices from other tools we'll end up constantly chasing any changes in those tools / conventions. Best to describe those other conventions in one place only (over in the repo for those tools) and keep it DRY. In any case, the discussion should really go in a new issue.
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 should just accept and move that verb tense convo elsewhere. I don't feel there's any consensus on past vs present -- there's a strong argument for "past" in the link below fwiw.
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.
Created stub issue: #42
index.md
Outdated
@@ -37,6 +37,30 @@ consumers of your library: | |||
A scope may be provided to a commit's type, to provide additional contextual information and | |||
is contained within parenthesis, e.g., `feat(parser): add ability to parse arrays`. | |||
|
|||
## Examples | |||
|
|||
### Commit message with body and breaking change |
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.
### Commit message with body and breaking change
Should be:
### Commit message with description and breaking change in footer
because with commit body
we mean the section between the footer and the type + description :D
d684c53
to
197c1eb
Compare
@damianopetrungaro thanks for the review and I've made the changes you requested. Hopefully this is ready to be merged now. |
@@ -37,6 +37,34 @@ consumers of your library: | |||
A scope may be provided to a commit's type, to provide additional contextual information and | |||
is contained within parenthesis, e.g., `feat(parser): add ability to parse arrays`. | |||
|
|||
## Examples | |||
|
|||
### Commit message with description and breaking change in body |
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.
change in footer*
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.
Please correct me if I'm wrong, but there is no footer. There is only a body.
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.
Is there anything holding this up from being merged? I think I've made all the requested changes. |
@tchaffee please tag owner of the repo 😄 |
@damianopetrungaro Do you know who that is? |
|
||
see the issue for details on the typos fixed | ||
|
||
fixes issue #12 |
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 is typically Closes: #12
. The body above should start with caps.
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.
@blackxored those aren't on the specs.
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.
They might not be, but I think we should strive for good commit messages as examples, and this isn't one IMHO, at least for the examples providing one that wouldn't clash with good practices (for ex. commitlint and config-conventional which people will learn to associate with this even if they're not) might be good.
In that spirit, I'd replace the body to be explicit (it's meaningless in its current form) and make the footer just Closes: #12
.
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 may agree with you, but if isn't specified in the specs it won't be used.
You can open another PR and propose those changes and discuss it, but atm those are just your usage of the specs.
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.
Hmm, as I've pointed out in #21 (comment) I think it's a natural step for people to refer to @commitlint/config-conventional while adopting this spec, it's not just my usage but what's expected by the ecosystem I reckon, commitlint would error at these commit messages, and it's counterintuitive IMHO. Thoughts?
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.
The point is:
If we want to add this to the spec we should open issues -> discuss it -> create PR improving the specs.
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.
Since my examples follow the spec, I think they should stay as-is and be merged and another issue should be opened to discuss whether or not the examples should demonstrate more than the spec and why. I typically don't like examples to include anything other than exactly what the spec requires. If the spec is missing some best practices, the spec itself should be improved, and that's a separate discussion.
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.
Yup, avoiding to block a PR and to delay the merge for discussions.
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.
@blackxored In case it sounded like people were being pedantic or difficult, I think it's just that they wanted us all to exercise the spec. If something feels "right", it should be codified in spec before changing behaviour :)
It's perhaps hardline, but an excellent way to keep the spec moving along!
Selfless plug: if you need more examples, which are valid for tools, take a look at this section on a post I wrote. |
@bcoe Any feedback here? I'd like to get this closed out and move on before I get distracted again with other projects and can't revisit this for a while. |
Ping. Any chance we can get this merged? |
@patcon please, move to another issue, this is not the right place. |
@damianopetrungaro @patcon would definitely love more maintainers chipping in. Full disclosure, my wife passed away at the start of 2018 and it's been taking me a while to get back into open-source again -- hoping I'll be a bit more on the ball again in the near future. I have a slack I've been managing for open-source projects like conventional commits: http://devtoolscommunity.herokuapp.com/ This is a good place to come have a real-time conversation about better maintaining this projects (and to express an interest in being a maintainer). |
oh man, I'm really sorry for your loss. |
@bcoe I'm so sorry to hear that. It can take a long time to get back to anything even resembling a normal routine. I'm also happy to help out in any way I can. |
@bcoe So sorry to hear about this man, very sorry for your loss. |
@patcon @tchaffee @blackxored thanks for your thoughts 👍 I'm happy to add more folks to the project, I think we all have the same goals: getting the language and rules around conventional commit messages as straight-forward as possible, and making sure the other libraries in this repo match those rules. Private message me in http://devtoolscommunity.herokuapp.com/, and I'll get a few more folks added as contributors. |
I was pointed to the specification and wanted to just get started, which would have been possible with examples. Instead I did a lot of reading about the motivation for the spec, and the formal spec itself. I am hoping the examples will help other people see how simple the spec is and to just get started quickly.