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

feat: add support for setter prefix and suffix #94

Merged
merged 9 commits into from
Jun 19, 2023

Conversation

Techassi
Copy link
Contributor

@Techassi Techassi commented Jun 16, 2023

This PR adds support to add prefixes and suffixes to setter methods.

#[derive(TypedBuilder)]
#[builder(field_defaults(setter(prefix = "with_", suffix = "_value")))]
struct Foo {
    x: i32,
    y: u32,
}

let foo = Foo::builder().with_x_value(1).with_y_value(2).build();

@mo8it
Copy link
Contributor

mo8it commented Jun 16, 2023

May I ask why? It is just more to type repeatedly :/

@Techassi
Copy link
Contributor Author

It is nice to have the option. For example I personally like to prefix my (manually implemented) builder methods with with_.

This is no breaking change and users don't have to set these values. If they are not set, the setter method is named like the field (current behavior). So really users can choose if they want to type more.

@mo8it
Copy link
Contributor

mo8it commented Jun 16, 2023

I see your point, but I think that this is subjective and adds complexity to the code. The whole crate and all its builder attributes have the purpose to type less.

I think that it is fine for a crate to be opionated to some degree.

But let's see what @idanarye thinks about it.

BTW: Always open an issue and wait for the response of the maintainers before starting with a PR. I was disappointed enough times by rejected PRs because they don't meet the goals of the maintainers :)

@idanarye
Copy link
Owner

While I myself don't see the point in using set_ or with_ prefixes in a builder that does not grant public access to the internal fields, Typed Builder already allows customizing a lot of things so I don't see a point to get opinionated about this thing in particular. And while it does add complexity being a new feature and all, I think that complexity is very localized.

typed-builder-macro/src/struct_info.rs Outdated Show resolved Hide resolved
typed-builder-macro/src/struct_info.rs Outdated Show resolved Hide resolved
@idanarye
Copy link
Owner

Please add a test with a keyword identifier for a field. Typed Builder had a problem with those in the past (#47), and I suspect this new feature (especially the prefix) may choke on it as well.

@idanarye
Copy link
Owner

Also, I'm not sure how I feel about the prefix and the postfix automatically adding the underscore.

@idanarye
Copy link
Owner

And please add an entry to the changelog.

@Techassi
Copy link
Contributor Author

Also, I'm not sure how I feel about the prefix and the postfix automatically adding the underscore.

Removed the underscore in cc1c5da

And please add an entry to the changelog.

Updated in fbae931

@Techassi
Copy link
Contributor Author

Please add a test with a keyword identifier for a field. Typed Builder had a problem with those in the past (#47), and I suspect this new feature (especially the prefix) may choke on it as well.

Added and fixed in ce48427

@Techassi Techassi requested a review from idanarye June 19, 2023 09:28
src/lib.rs Outdated Show resolved Hide resolved
typed-builder-macro/src/util.rs Outdated Show resolved Hide resolved
typed-builder-macro/src/field_info.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@Techassi Techassi requested a review from idanarye June 19, 2023 11:05
typed-builder-macro/src/field_info.rs Outdated Show resolved Hide resolved
@Techassi Techassi requested a review from idanarye June 19, 2023 11:18
@idanarye idanarye merged commit 4aadd75 into idanarye:master Jun 19, 2023
8 checks passed
@Techassi Techassi deleted the feature/method-prefix branch June 19, 2023 13:39
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.

3 participants