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.

Sign up for GitHub

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

Jump to bottom

Consuming build but by mut setters? #321

Open
VorpalBlade opened this issue Jun 10, 2024 · 4 comments · May be fixed by #327
Open

Consuming build but by mut setters? #321

VorpalBlade opened this issue Jun 10, 2024 · 4 comments · May be fixed by #327

Comments

@VorpalBlade
Copy link

VorpalBlade commented Jun 10, 2024 •
edited
Loading

It seems #[builder(pattern = "owned")] changes the behaviour of both the build function as well as the setters. I would like to decouple that:

  • I have a builder that I build in a loop parsing a section + key/value file. For this is is more convenient to have &mut setters. Additionally I'd like to stick the builder in an Option to more easily handle the case of the first section / empty file. Consuming setters are quite unwieldy in that case.
  • Some of the fields are quite heavy Vecs and I won't reuse the builder for the next section (each section is a clean slate). As such I would like a consuming build to avoid clones.

The pattern looks something like this in pseudo-rust:

let mut results = vec![];
let mut builder = None
let mut buffer = String::new();

while input.read_line(&mut buffer)? > 0 {
    let line = buffer.trim();
    // Handle new section (which also has the name of the section in it)
    if let Some(stripped) = line.strip_prefix("EntryThatIndicatesNewSection: ") {
        if let Some(inner) = builder {
            results.push(inner.build());
        }
        builder = Some(Builder::new());
        builder.as_mut().unwrap().name(stripped)
    } else if let Some(stripped) = line.strip_prefix("OtherEntry: ") {
        // Handle other fields, some are mandatory, some are optional (have defaults on builder)
        builder.as_mut().unwrap().other_entry(stripped);
    } else if let Some(stripped) = line.strip_prefix("AnotherEntry: ") {
        // ...
    }
    buffer.clear();
}

// Handle final entry
if let Some(inner) = builder {
    results.push(inner.build());
}

This is leaving out proper error handling etc for clarity, but it is there in the real thing.
@VorpalBlade
Copy link
Author

By the way, I checked with profiling and in my case the clone calls for those Vecs do not get optimised away unfortunately.

@TedDriggs
Copy link
Collaborator

This should be straightforward to implement.

API

We'd allow setting the pattern in #[builder(build_fn(pattern = "..."))]. if present, this would override the one set at the #[builder(pattern = "...")] level; if absent, the builder-level pattern will be used.

Implementation

Options::as_build_method would be updated to use the new build_fn override when initializing BuildMethod's pattern field.

TedDriggs added a commit that referenced this issue Jul 16, 2024
@TedDriggs
Allow setting build_fn-specific pattern
2a79a0a 
This allows a builder to have mutating setters and an owned build method, removing the need to do clones during construction.

Fixes #321
@TedDriggs TedDriggs linked a pull request Jul 16, 2024 that will close this issue
Open
3 tasks
@TedDriggs
Copy link
Collaborator

Looking at this again, I'm now second-guessing my original thought on the API. It feels like it might be better for the top-level pattern field to be the one that controls the signature of the build function, while allowing the override of the setters in the #[builder(setter(pattern = "..."))] meta item at the struct level.

@TedDriggs
Copy link
Collaborator

Okay, so I think the reason this is proving more challenging than expected is that the current behavior of the pattern field is weird.

Currently, that field can be set on the struct or on a field. The strange part is how setting it on a field interacts with the build method. I don't understand the rationale behind the current design, so I'm hesitant to change it too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow setting build_fn-specific pattern colin-kiegel/rust-derive-builder
2 participants
@VorpalBlade @TedDriggs