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

fix: enforce a lower case letter at the beginning of the summary content #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aeyoll
Copy link

@aeyoll aeyoll commented Jan 3, 2022

No description provided.

@aeyoll aeyoll force-pushed the summary-content-starting-with-lowercase branch from ea0f755 to f9a6fc6 Compare January 3, 2022 18:47
@cocogitto-bot
Copy link

cocogitto-bot bot commented Jan 3, 2022

✔️ f9a6fc6 - Conventional commits check succeeded.

@aeyoll aeyoll changed the title fix: enforce a lower case character at the beginning of the summary content fix: enforce a lower case letter at the beginning of the summary content Jan 3, 2022
@oknozor
Copy link
Collaborator

oknozor commented Jan 3, 2022

Hey @aeyoll I am a bit confused, is this mentioned in the conventional commit spec ?

@aeyoll
Copy link
Author

aeyoll commented Jan 4, 2022

Hey @oknozor,
It's not per se in the conventional commit spec, but it's present in the angular commit message spec and tools like commitlint is enforcing it by default.

I understand that it can be out of scope for this project if its not described in the spec.

However, maybe it could be a configurable option in the parser?

(My goal behind all this is to ditch commitlint and replace it with cog verify in all my repositories, where the "lowercase" rule is already in place)

Thanks a lot

@oknozor
Copy link
Collaborator

oknozor commented Jan 4, 2022

Hey @aeyoll,

Thanks for the suggestion but I cannot merge this as is.
Adding angular commit to cocogitto as an additional layer on top of the conventional commit spec seems ok to me.
That said, breaking the current implementation ( which is conform to the spec ) is not an option here.

However, maybe it could be a configurable option in the parser?

This is the way to go. Ideally we could expose a parser builder which would allow to add more parsing options without breaking the public API again.

Also adding the lowercase rule only is not ideal. I'd rather have the angular commit spec implemented all at once.

Regarding the implementation I think the pest grammar file for the conventional commit spec should be left untouched.
It describes the conventional commit parser and should not be mixed with angular commit.
Their are few additional rules a second pass on the already parser conventional commit would do. I am not even sure
pest is needed here.

Once we have this in the parser I would be trivial to add an angular commit flag/config to the cli.

Let me know if you want to work on it.

@aeyoll
Copy link
Author

aeyoll commented Jan 4, 2022

Thanks for the feedback, I totally understand that you can't merge it.

Also adding the lowercase rule only is not ideal. I'd rather have the angular commit spec implemented all at once.

The Angular spec is actually not that different from conventional commit spec, I think it only adds this lower case rule, plus a max length of 100 of every line of the message.

Ideally we could expose a parser builder which would allow to add more parsing options without breaking the public API again.
Their are few additional rules a second pass on the already parser conventional commit would do. I am not even sure
pest is needed here.

It would be the easiest way, but it adds another layer of verification on top of pest, I'm not sure if it's the most elegant solution. We could have multiple grammar files, but they would have a lot of similarities, which is not ideal either. Too bad pest doesn't allow to share rules between multiple grammars.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants