Refactoring Ideas

[colin-kiegel]

Here are some refactoring ideas for the codebase:

• reexport macro to allow exporting additional items refactoring: re-export custom derive with additional items (WIP) #94
  • refactor no_std implementation (turn no_std attribute into a feature flag?)
  • extend validation feature with Validate trait
  • definitions for structured errors?
  • ,,,
• remove the foo_enabled flags from derive_builder_core objects. Instead just
  don't emit the objects. ;-) I.e. change options.as_foo -> Foo<'a> to
  options.as_foo -> Option<Foo<'a>>?
• TryFrom delegates to the build function. In the future I would like to
  reverse that. But let's wait until TryFrom is stable.
• Simplify processing of attributes / structured MetaItems
  Simplify processing of structured MetaItems dtolnay/syn#25
  • Simplify processing of structured MetaItems dtolnay/syn#25 (comment)
  • prom-attire: https://github.com/Nemo157/prom-attire-rs/
• ...

Maybe later:

• get rid of logging? We didn't need it so far..

cc @TedDriggs

[TedDriggs]

refactor no_std implementation (turn no_std attribute into a feature flag?)

How is the library returning String for errors in no_std mode today? I can't compile something with String when #![no_std] is in use.

extend validation feature with Validate trait

Maybe I'm missing something, but I don't get why this would need to be a trait. The validate attribute with no path could look for an inherent method called validate just as easily as it could look for a trait method, and an inherent method gives the author much more control over the function's visibility.

remove the foo_enabled flags from derive_builder_core objects. Instead just
don't emit the objects. ;-) I.e. change options.as_foo -> Foo<'a> to
options.as_foo -> Option<Foo<'a>>?

The BuilderField type still emits something when disabled. I considered making this optimization when I was writing from_variants, but I ended up reversing course; see TedDriggs/from_variants#4 (specifically this line). I found it was easier to keep track of everything until the last moment than trying to optimize by returning None early.

Simplify processing of attributes / structured MetaItems

❤️ ❤️ ❤️ this looks amazing.

[colin-kiegel]

@TedDriggs Thx for the feedback. :-)

How is the library returning String for errors in no_std mode today? I can't compile something with String when #![no_std] is in use.

We depend on ::collections:: string::String then and the user must import extern crate collections.

Maybe I'm missing something, but I don't get why this would need to be a trait. The validate attribute with no path could look for an inherent method called validate just as easily as it could look for a trait method, and an inherent method gives the author much more control over the function's visibility.

Yeah, the visibility thing is not optimal. But my primary motivation would be better error messages error: `FooBuilder` does not implement `derive_builder::Validate` instead of very weird type errors without any source code highlighting, if the validate function just returns the wrong type, etc. Also the collision risk is reduced (say if there is a setter called validate).

The BuilderField type still emits something when disabled. I considered making this optimization when I was writing from_variants, but I ended up reversing course; see TedDriggs/from_variants#4 (specifically this line). I found it was easier to keep track of everything until the last moment than trying to optimize by returning None early.

Ok, I'll have a look.

Simplify processing of attributes / structured MetaItems

❤️ ❤️ ❤️ this looks amazing.

Note: I'm not yet 100% sold on prom-attire. I could imagine implementing a custom parser based on synom instead in order to have more control about the process and the way we can report errors. Before investing too much effort on migrating to prom-attire, I'd suggest we try a very small proof-of-concept first - like a parser for only some #[builder(setter(...))] attributes. This would allow us to study how things like error reporting and overwriting struct defaults with field settings would look like.

[colin-kiegel]

PS: I also like to pull in as little dependencies as possible and we already depend on synom via syn. However prom-attire would pull-in error-chain and it's dependencies.

[TedDriggs]

I've gotten the tests working on my fork of your branch, but I don't know how to push that to the one you sent me a PR for. I'll investigate prom-attire on a separate fork. Flagging @Nemo157 about error-chain; I have some concerns about it too because I think it takes a hard dependency on String for the automatically-added Msg variant.

[TedDriggs]

My initial experiments with prom-attire aren't going well:

1. I don't see how to work with nested lists, like #[builder(setter(into, name = "hello"))]; if I try to make a sub-struct for the Setter options, it complains that there's no FromStr impl on the sub-struct.
2. There isn't support for generic types, which is how OptionsBuilder<Mode> works today. If there was a way to provide callback paths on certain fields, I can think of a way to make this work but it's not there now.
3. Fields' types are required to implement std::str::FromStr for name-value pairs. This would cause problems for our builder properties that are from the syn crate, such as Visibility.
4. Related to the item above, the FromStr::Err type must implement std::error::Error.

[Nemo157]

I can easily drop the error-chain dependency from prom-attire, quick-error should do everything that prom-attire needs. Although, what's the issue with having a dependency on String?

I don't see how to work with nested lists, like #[builder(setter(into, name = "hello"))]; if I try to make a sub-struct for the Setter options, it complains that there's no FromStr impl on the sub-struct.

That's not a pattern I'd thought of using, it should definitely be possible to support something like that though. At the moment it has special-cased support for literal types and everything else goes through FromStr. I have been thinking that it really should be using its own trait for this instead, if it passed the attribute through instead of a string value then that could pass down into a sub-parser. Although that might require having a runtime library 😦

There isn't support for generic types, which is how OptionsBuilder<Mode> works today. If there was a way to provide callback paths on certain fields, I can think of a way to make this work but it's not there now.

I've only taken a brief look at the derive-builder codebase, but from what I think you're talking about here you have a base struct OptionsBuilder that covers the combined subset of attributes that apply to the struct or each field, then pass in a different mode depending on the current context?

It seems that if prom-attire is able to take care of all the parsing nicely, then the duplication of having two structs with a lot of shared fields, but some different, is not necessarily a bad thing. Otherwise I think I need to look at the derive-builder code a bit more and see how you're currently parsing the attributes.

Fields' types are required to implement std::str::FromStr for name-value pairs. This would cause problems for our builder properties that are from the syn crate, such as Visibility.

Well, how do you parse a syn::Visibility today? I actually really wish that syn had FromStr implementations for all its types. The current way to achieve this is to have a new-type around syn::Visibility just to implement FromStr for it, I have contemplated adding in a way to pass in an alternative parsing function as well.

Related to the item above, the FromStr::Err type must implement std::error::Error.

Any reason not to? prom-attire needs FromStr::Err : std::fmt::Display at the very least so it is able to provide useful error messages when parsing fails. If you have a good argument against it I guess it could be turned off via an option.

[TedDriggs]

I actually really wish that syn had FromStr implementations for all its types.

I submitted dtolnay/syn#131 today to address just that.

I've started parallel work on darling, which is trying to achieve the same goal as prom-attire but leveraging some of the tricks I've learned from derive_builder and from serde. I'd be open to merging my work with prom-attire, but I found I needed a clean slate to properly explore the idea.

[TedDriggs]

Closing this in the interest of ticket hygiene; we've merged a significant simplification of options parsing, and the other items can/should be ticketed on their own.