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

Builders for options #174

Closed
wants to merge 3 commits into from
Closed

Builders for options #174

wants to merge 3 commits into from

Conversation

kivikakk
Copy link
Owner

I keep breaking semver compatibility by adding options. Change public-facing options to builders so adding an option is backwards-compatible.

I'm not super sold on this variety of builder, namely the way you need to call options.x for several xs if you want to modify multiple kinds. Maybe I could roll all the builder functions into one impl.

@YJDoc2
Copy link
Contributor

YJDoc2 commented Oct 23, 2021

Hey, If you're still interested in adding a builder implementation, what do you think of using https://crates.io/crates/derive_builder for this? That will generate the builder automatically, and one won't have to remember to specifically add a corresponding builder function. Also for the semvar-breaking issue, what do you think of adding non-exhaustive attribute to the options structs? That way they cannot be manually instantiated, and won't break others depending on them.

@kivikakk
Copy link
Owner Author

Oh yeah, that seems way better. Making them non-exhaustive also seem really good; possibly could just go with that instead entirely. I don't really have the time in life to make either of these happen myself at the moment, but would certainly appreciate help going in either direction.

@YJDoc2
Copy link
Contributor

YJDoc2 commented Oct 24, 2021

Hey, I would like to help and maybe send a PR if you'd be willing to review it and accept it. Let me know.
The builder option would provide an easier way to build options rather than having type each of the struct option/ use ... . The non-exhaustive would allow extending the options without breaking the depending crates, as it would force them to have a _ option in match. Both seem a good thing to add. Let know which ones would you like to add.

@kivikakk
Copy link
Owner Author

@YJDoc2 Sorry I never replied to your comment! Life got a lot.

I'd be very happy to accept such a PR. Let me know if this is something you still have any interest in taking on — and I understand it might not be, of course, given the passage of time — otherwise I'll likely get to it soon while tidying up Comrak.

@YJDoc2
Copy link
Contributor

YJDoc2 commented Mar 29, 2023

Hey, no worries! 😄

I am still interested in helping out, but I'll need to take another look at what I was suggesting as I seem to have forgotten that myself 😂

Also, I am a bit busy with other stuff at the moment as well, so how about this :
I will try to get a PR opened up as I can, that adds the suggested builder stuff ; but in case you get to tidying up before it, feel free to continue with it and close this as appropriate.

In case you need a more concrete timeline, let me know, I'll try to get it done sooner, or just pass on it 👍

@YJDoc2
Copy link
Contributor

YJDoc2 commented Mar 29, 2023

@kivikakk Opened #292 , please take a look!

@kivikakk kivikakk closed this Mar 31, 2023
@kivikakk kivikakk deleted the builders branch March 31, 2023 07:56
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