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

setter(custom) which allows a custom setter implementation #154

Merged
merged 1 commit into from
Nov 7, 2019

Conversation

jgraef
Copy link
Contributor

@jgraef jgraef commented Nov 2, 2019

Implements feature request #110

First I added an option to the FieldLevelSetter called custom. Such that the attribute is #[builder(setter(custom))].

This option will behave like skip but not cause the field to be "skipped" (i.e. replaced with PhantomData). This is done by splitting the enabled flag into setter_enabled and field_enabled. custom will only cause the setter to be disabled, but not the field.

I'm aware that custom is missing for StructLevelSetter. To be honest, it's late and I need the feature. I can implement it tomorrow, if necessary :)

Tests are implemented in tests/setter_custom.rs.

I just started using this crate today and didn't look to deep into the code and it's capabilities (doc seems to be lacking). So there might be some inter-dependencies I overlooked. I copied the tests from skip-setters.rs and adapted them. I also added a test for strip_option as this might be a common usecase together with custom.

If there are any recommendations for improvements, let me know.

@jgraef
Copy link
Contributor Author

jgraef commented Nov 2, 2019

It just came to mind that the strip_option doesn't make sense in combination and the attribute can be omitted since the setter is custom anyway. I guess it doesn't hurt to have the test anyway.

@jgraef
Copy link
Contributor Author

jgraef commented Nov 7, 2019

I didn't follow up on the struct level attributes. I'm quite busy right now, but can do it at a later time.

I know this crate is kind of unmaintained or at least looking for a maintainer - which I don't get since builders are a thing that are widely used in the Rust ecosystem. How are the chances this get's merged soonish? We can just depend on a git for now, but would want to release our software to crates.io at some point, where we need a proper dependency.

I'm willing to offer some of my time to help maintain this project, if that's needed. Just a really quick feedback on this would be nice :)

Copy link
Collaborator

@TedDriggs TedDriggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think this is going to require we bump to 0.9.0 since it changes the crate's public interface. Outside of this PR, I think it's worth looking at a refactor which reduces the inter-crate API surface we expose; having to do a minor version bump for this is unfortunate.

@TedDriggs TedDriggs merged commit b4d8e69 into colin-kiegel:master Nov 7, 2019
@jgraef
Copy link
Contributor Author

jgraef commented Nov 7, 2019

Do I understand this correctly? It's a minor version bump because the interface of FieldLevelSetter changed? I didn't even consider that to be a public interface, but yeah of course it is.

But if I undo the renaming of the enabled method, this shouldn't require a minor version bump, right?

@TedDriggs
Copy link
Collaborator

You're correct. However, I think your name is better so I've published with the bump. I'll file a separate issue to refactor in the future.

@jgraef
Copy link
Contributor Author

jgraef commented Nov 7, 2019

Nice.

A struct level attribute would also require a breaking change then. But does this attribute make sense on a struct level? The struct level attribute would basically disable all generated setter implementations. But you could always overwrite it with the field level attribute again.

Anyway, a refactor would be a good idea. I was also thinking about implementing a few other feature requests, since some of them seem pretty easy :) But I'll better wait for the refactoring then.

@stoically
Copy link

Would be great to have this useful feature documented as I was only able to find it by searching here on github.

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.

None yet

3 participants