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

Core API being provided through traits isn't very ergonomic #17

Open
epage opened this issue Nov 16, 2019 · 4 comments
Open

Core API being provided through traits isn't very ergonomic #17

epage opened this issue Nov 16, 2019 · 4 comments

Comments

@epage
Copy link
Contributor

epage commented Nov 16, 2019

Traits work best when multiple independent implementations might exist and code wants to operate on them. However, this API uses traits to provide two different flavors of the same API without having conflicting or weirdly named symbols. However, this approach requires the to use the trait to do anything.

I'd recommend just getting rid of the stringly typed API. I don't think it will be a big loss

  • Stringly-typed APIs are generally frowned upon in Rust so there won't be too much expectation to have one
  • We implement a lot of different traits for people to access the actual str.

One open question in my mind is if Description, Body, and maybe FooterValue provide any extra feature or safety value by using a newtype.

@mainrs
Copy link

mainrs commented Jul 25, 2020

One open question in my mind is if Description, Body, and maybe FooterValue provide any extra feature or safety value by using a newtype.

Could you elaborate on this more? I am currently looking for a parser that can parse conventional commits but I really dislike the API based on traits (as you said). I might even write one myself as I couldn't really find a crate that I liked.

@epage
Copy link
Contributor Author

epage commented Jul 25, 2020

Maybe check out my fork at https://crates.io/crates/git-conventional

I was invited to help maintain this repo but with some delays in getting the invitation and in issues setting up the required MFA, I've just been working out of the fork, for now.

@mainrs
Copy link

mainrs commented Jul 25, 2020

Ah, good to know! I have been looking for some parsers and did start to work on my own. I couldn't find something that is generic enough over the input. All of them had the commit types hard-coded, which conflicts with the specification, as they can be anything.

I wish I had seen your repository earlier. I am almost done though. The problem that I faced was that out of all libraries, only one did zero-copy parsing. This was one implementation detail that I definitely wanted. Your library looks good and the only nitpick I have are probably personal coding style differences. For example, I am not 100% sure why you used delegate methods instead of making the struct fields public. That way, someone can use the same struct and alter the message if they need to do so.

If you are interested, I am starting to fork/re-implement the whole conventional-commits ecosystem in Rust under the @conventional-commits-rs organization. Some stuff is still on my current account, for example git-cm. But those will move over eventually.

@epage
Copy link
Contributor Author

epage commented Jul 30, 2020

For example, I am not 100% sure why you used delegate methods instead of making the struct fields public.

Mainly so I could change the internals without breaking people. There aren't too many internals that can change though.

If you are interested, I am starting to fork/re-implement the whole conventional-commits ecosystem in Rust under the @conventional-commits-rs organization.

I think it'd be great if we worked together. I don't know enough about the ecosystem to know how worth it it is to recreate their stuff vs integrate with existing rust stuff, for example

  • I have committed for validating commits which includes conventional style but isn't exclusive to it
  • Having support for inferring the level to release a crate at would be great for cargo release

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

No branches or pull requests

2 participants