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

Possibly replace git-conventional with conventional-commit-parser #28

Closed
oknozor opened this issue Oct 21, 2021 · 6 comments
Closed

Possibly replace git-conventional with conventional-commit-parser #28

oknozor opened this issue Oct 21, 2021 · 6 comments
Assignees
Labels
feature/request New feature or request

Comments

@oknozor
Copy link

oknozor commented Oct 21, 2021

Is your feature request related to a problem? Please describe.

Hi @orhun

This is not directly related to git-cliff, as you know I am refactoring the cocogitto changelog logic to use git-cliff internals.

Now I am facing the following dilemma :
cocogitto uses conventional_commit_parser, git-cliff uses git-conventional which does exactly the same.
In order to integrate git-cliff I need either to use git-conventional and drop conventional_commit_parser or do the opposite in
git-cliff.

Describe the solution you'd like
I am biased toward the library I wrote but I think it has a few cons :

  • Slightly smaller crate size ( 11.3 kB vs 14kB)
  • Pest grammar is easy to read, so probably easier to maintain
  • I removed all string allocations from the latest version to improve perfs and look more like git-conventional API

If you are ok with this I can make a draft PR in git-cliff.

Describe alternatives you've considered

If you have some good reason to stick with git conventional I'll refactor on cocogitto's side to get this done. Let me know.

@oknozor oknozor added the feature/request New feature or request label Oct 21, 2021
@orhun
Copy link
Owner

orhun commented Oct 22, 2021

I had a look at your library and I think git-conventional and conventional-commit-parser are both decent libraries for parsing conventional commits. (Using pest is a good idea) However, in this case, I'd like to stick with git-conventional because it has serde support and I personally think @epage is doing a good job maintaining the library. He was very responsive and helpful about the issues that popped up regarding git-cliff in the past months.

Let me know if I can help with anything else :)

@epage
Copy link

epage commented Oct 22, 2021

@oknozor Any thoughts on collaborating?

I will admit, I've had bad experiences in the past with pest and much prefer nom. Unsure if that will be a deal breaker or not. I am working with the nom maintainers to improve its readability.

@oknozor
Copy link
Author

oknozor commented Oct 22, 2021

@epage I'd be glad to contribute if needed, but I have never used nom before. Also regarding your experience with pest this issue might be interesting.

When I made conventional-commit-parser I was not aware about git-conventional. I have just made a quick poc to migrate from git-conventional to conventional-commit-parser. Turns out the API is 95% similar (you can see the diff here), so like I said before I would be happy to drop conventional-commit-parser and use git-conventional.

There is one thing that puzzle me though : In the migration attempt I made the main test is failing :

Diff < left / right > :
<"# Changelog\n## Unreleased\n\n### Bug Fixes\n#### app\n- fix abc\n\n### New features\n#### app\n- add xyz\n\n### Other\n#### app\n- document zyx\n\n#### ui\n- do boring stuff\n\n## Release [v1.0.0] - 1971-08-02\n(0bc123)\n\n### Bug Fixes\n#### ui\n- fix more stuff\n\n### New features\n#### app\n- add cool features\n\n#### other\n- support unscoped commits\n\n### Other\n#### app\n- do nothing\n\n#### ui\n- make good stuff\n------------"
>"# Changelog\n## Unreleased\n\n### Bug Fixes\n#### app\n- fix abc\n\n### New features\n#### app\n- add xyz\n\n### Other\n#### app\n- document zyx\n\n#### ui\n- do boring stuff\n\n## Release [v1.0.0] - 1971-08-02\n(0bc123)\n\n### Bug Fixes\n#### ui\n- fix more stuff\n\n### New features\n#### app\n- add cool features\n\n### Other\n#### app\n- do nothing\n\n#### ui\n- make good stuff\n------------"

All commit are parsed correctly but one is not displayed in the final output, I was not able to find why. I'd be curious to know what is causing this. @orhun any idea ?

I will try switching to git-conventional on cocogitto's side and let you know how it goes.

@epage
Copy link

epage commented Oct 22, 2021

That describes the biggest issue I have with pest (my equivalent post for nom). The slightly less important one is I'm not happy with is the compile times from being a proc-macro doing code-gen.

@orhun
Copy link
Owner

orhun commented Oct 23, 2021

All commit are parsed correctly but one is not displayed in the final output, I was not able to find why. I'd be curious to know what is causing this. @orhun any idea ?

It seems like this commit is not processed:

feat: support unscoped commits

Support for these types of commits was added with #8. You can check out the implementation here. Maybe there is an API difference between two libraries.

edit: this change seems like the issue: oknozor@7088bc6#diff-fb8ea8d3a2eea059cc2540308b82d45fd5194ec545a113394ea3a75bcba3bad1R148

@oknozor
Copy link
Author

oknozor commented Oct 27, 2021

I think you can close this now, just so you now I tried integrating both git-conventional and the latest conventional-commit-parser in cocogitto.
Took me a while (and it was painful) to adjust lifetime bounds to make cocogitto work with git-conventional, the cocogitto code base ended less readable with almost no perf improvement, so I have decided to switch back to previous version of conventional-commit-parser that returns owned value.

@oknozor oknozor closed this as completed Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/request New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants