colin-kiegel / rust-derive-builder

derive builder implementation for rust structs
https://colin-kiegel.github.io/rust-derive-builder/
Apache License 2.0
1.28k stars 82 forks source link

Added builder() fn for the original struct which returns the actual builder #265

Closed alexzanderr closed 1 year ago

alexzanderr commented 1 year ago

Hello.

Love your crate, thanks for the code.

Here I've added just a simple builder() function for the original struct, this is a small PR, but I really want that change in the original crate, to not make wrappers.

What I've changed in this PR:

Additional notes:

Thats it. I assuming that you won't like the code structure, it didnt follow your pattern, we'll see. In the first place I tried to add the changes to the to_tokens function, but I realised that this function doesnt get ast: syn::DeriveInput as parameter and also Builder struct has no fields with ast, has everything except the name of the original struct. I really needed the name of the struct, the name of struct wasn't defined in the to_tokens function, so I made a separate method for Builder to call from the macro.

Anyway, I'm open to code suggestions if you dont like it.

Thanks.

alexzanderr commented 1 year ago

Thanks a lot for the response!

Yeah .. I have a lot to work in order to fix these.

I will try to apply the all of the above suggestions and I will come back with updates on this PR or a make new PR.

TedDriggs commented 1 year ago

I will try to apply the all of the above suggestions and I will come back with updates on this PR or a make new PR.

Please note that addressing the review comments won't address the larger objections raised here and in #170, so I still wouldn't anticipate this functionality being merged.

alexzanderr commented 1 year ago

Sure I see your point.

But if I fix all the review problems and try to make this work without any problems and without any messes, would you approve it?

I mean, there is a big IF.

But woulnd't you approve it if hypothetically I can make this work?

TedDriggs commented 1 year ago

But if I fix all the review problems and try to make this work without any problems and without any messes, would you approve it?

Unlikely. The problems noted in #73 aren't something that any implementation can fix; there's simply no way to make this an option of the derive-macro and avoid this method being in a second impl block, which messes up the docs for the deriving type. And - as discussed in #170 - it's not clear what we'd name the flag for something like this.