Vision proposal #79

TedDriggs · 2017-04-14T17:31:31Z

Here's the vision proposal doc related to discussion of #67. @colin-kiegel and @faern, you should now be able to add inline comments to the proposal.

EDIT: Rendered.

- move all integration tests into `derive_builder` crate - shrink `derive_builder_test` crate into `tests/dummy-crate`

colin-kiegel

Thank you very much for starting this general discussion and your effort! 👍

I've left a few inline comments.

TL;DR

I still have some concerns with indirect complex derives like serde, which we should discuss in Add derives on derived FooBuilder struct #43.
In terms of general direction, I'd prioritize static validation of required fields and integrated validation of values higher than the kind of flexibility suggested in this proposal. But hopefully we can still work out some compromises. :-)

I hope it doesn't sound too critical - I think it's good to see and discuss these kind of tensions in the design space. So thank you again. :-)

colin-kiegel · 2017-04-15T13:56:26Z

vision_doc.md

@@ -0,0 +1,279 @@
+# Vision


We want to generate code that is as idiomatic as possible. Please add a corresponding statement and a reference to this unofficial design pattern.

colin-kiegel · 2017-04-15T13:56:30Z

vision_doc.md

+* Function arguments
+* Wire/file/database inputs
+
+The builder should be focused specifically on the assembly of inputs and validating the completeness of the parameter set; additional validation should be handled by a constructor function provided by the target type. That constructor function may be public or private, depending on whether or not the crate author wants to _require_ the use of the builder.


I can see your point in decoupling validation from being a builder only thing.

But how that validation happens is a detail IMO and should not be decided on this prominent level.

The vision should be very high level. For me a Builder (on a high level) is also about validation. If I have a PizzaOrder and Pizza object, then order.submit() should check if I can have 1 kg of tuna on one pizza. :-)

Everything beyond that seems to be to detailed for the entry paragraph of the vision for me.

I've moved it to a sub-section, but I do think that having some level of opinion on where validation fits in is significant. (Also, not sure where that statement would go otherwise as the doc stands now).

colin-kiegel · 2017-04-15T13:56:31Z

vision_doc.md

+## Documentation
+Authors should focus documentation efforts on the target type.
+* If the builder is the only way to create the target type, then the target type should discuss the requirements for its own creation.
+* The builder can automatically include a link to the target type doc HTML from its own struct-level doc comment.


(small sidenote: this link currently breaks if the target type name changes due to re-export under a different name.)

colin-kiegel · 2017-04-15T13:56:33Z

vision_doc.md

+* The builder can automatically include a link to the target type doc HTML from its own struct-level doc comment.
+
+## Imports
+A crate/module should always export the target type of a builder to allow the _built_ type to appear in function and struct type declarations. The exporting crate should _generally_ also expose the builder type, but may choose to exclude it from a prelude or the crate root, preferring to expose it in a child module. The crate is also free to keep the builder for its own internal use.


Ideally I would like to support both workflows. I think both have valid use cases. Could you perhaps change it to something less opinionated like: Both of these should be possible: ?

use FooBuilder; let foo = FooBuilder::default().foo(1).build()?;

use Foo; let foo = Foo::builder().foo(1).build()?;

If only FooBuilder is exposed, then the importing crate has no way to refer to the constructed type in structs or function signatures. I can't think of a case where that's desirable.

A possible usecase could be where you always just send the constructed value back into the library. A highly examplish example:

pub fn block_from_ip(ip: IpAddr) { let rule = firewall::RuleBuilder::default() .action(firewall::RuleAction::Block) .from(ip) .build() .unwrap(); firewall::add_rule(rule); }

But on the other hand the Foo::builder() approach works there as well and I don't see any situation where Foo::builder() would not work just as well as FooBuilder::default()

colin-kiegel · 2017-04-15T13:56:35Z

vision_doc.md

+## Imports
+A crate/module should always export the target type of a builder to allow the _built_ type to appear in function and struct type declarations. The exporting crate should _generally_ also expose the builder type, but may choose to exclude it from a prelude or the crate root, preferring to expose it in a child module. The crate is also free to keep the builder for its own internal use.
+
+For the case of builder population with statically-known values or through explicit construction from function arguments, the target type should (optionally) expose a static `builder()` method which returns the builder type. In addition to keeping the imports shorter, this will appear in RLS "find all reference" queries by type, will be updated automatically during type renaming within the workspace, and will show docs for the target type on type hover (requesting docs on the static method will get the user to the builder-specific documentation).


I don't understand this. Can you explain this?

Added links to the relevant RLS methods.

Ok, now I get it. You rename Foo to Bar and the RLS has you covered unless you explicitly named FooBuilder. Because that will be renamed implicitly to BarBuilder and the RLS will not know about that. :-)

colin-kiegel · 2017-04-15T14:06:05Z

vision_doc.md

+For the case of builder population with statically-known values or through explicit construction from function arguments, the target type should (optionally) expose a static `builder()` method which returns the builder type. In addition to keeping the imports shorter, this will appear in RLS "find all reference" queries by type, will be updated automatically during type renaming within the workspace, and will show docs for the target type on type hover (requesting docs on the static method will get the user to the builder-specific documentation).
+
+## Fallible vs. Infallible Builders
+The infallible-builder-with-required-fields case is interesting, but brittle; it can only be achieved by generating a custom constructor function which takes all required values up front, and creates a type which cannot be a drop-in replacement for a fallible builder.


This paragraph is more a discussion than a vision, but point taken. However I think it's an important use case to allow static reasoning about required fields (at compile time). This is also what typesafe builders #33 are about. IMO typesafe builders as described in #33 should be an essential part of the vision - and there's a lot left to be discussed IMO.

colin-kiegel · 2017-04-15T14:12:34Z

vision_doc.md

+1. Environment variables
+1. A TOML config file
+
+The union of the 3 sources must provide all the required values, or the `ConnectionPool` cannot be created. No individual source is required or expected to be complete.


nice use case. :-)

colin-kiegel · 2017-04-15T14:16:10Z

vision_doc.md

+            host: host,
+            api_key: api_key,
+            timeout: timeout
+        }


missing socket: socket,

colin-kiegel · 2017-04-15T14:33:44Z

vision_doc.md

+    ///   inherent `builder` function on the target type.
+    #[derive(Debug, Clone, Builder, Serialize)]
+    #[builder(setter(into), try_setter, preserve_attrs(serde), derive = "Deserialize", build_fn(skip), target_factory)]
+    pub struct StatRequest {


That looks complicated. I'm ok with #[builder(derive(Debug))] etc., but I'm still very sceptical about that level of indirection with complex derives like serde for different reasons stated in #43. We should continue the discussion about these things there.

Is your concern the preserve_attrs(serde) part, or the whole thing?

Actually a bit of both. That's a wall of attributes and it's hard to understand what this is doing. preserve_attrs(serde) and all those serde annotations also contribute significantly to that reasoning footprint.

colin-kiegel · 2017-04-15T14:45:55Z

vision_doc.md

+        fn try_from(v: StatRequestBuilder) -> Result<Self, Self::Error> {
+            v.build()
+        }
+    }


I feel like it's a bad sign, that you have to implement a constructor, a build method and TryFrom. That's a lot of boiler plate left. All that derive_builder does here is generate a secondary struct with getters and setters.

The TryFrom could be automatically generated if the user told us that the build method was implemented and what its name was, but I take your point. I was also trying to avoid any magic that depended on field order being the same in the struct declaration and the constructor, although something could be done with that which would make the code shorter.

I tried to pick an example where the validation logic was non-trivial and would need to be applied regardless of how the object was assembled.

[ci skip]

…lder into vision

colin-kiegel and others added 7 commits April 13, 2017 12:53

refactor workspace

573f3ae

- move all integration tests into `derive_builder` crate - shrink `derive_builder_test` crate into `tests/dummy-crate`

fix changelog

12941f5

CONTRIBUTING.md

d13d04a

Added doc comment explaining how OptionsBuilder gets used.

cbeb0ed

Vision proposal

d55c8ff

Merge branch 'refactor_workspace'

900c99e

cleanup and rustfmt

cd8e295

colin-kiegel reviewed Apr 15, 2017

View reviewed changes

colin-kiegel mentioned this pull request Apr 15, 2017

Custom validation of built struct #67

Closed

colin-kiegel and others added 6 commits April 15, 2017 22:02

cleanup

14f4bba

clippy

e48b24d

[ci skip]

Vision proposal

27475d3

Merge branch 'vision' of https://github.com/TedDriggs/rust-derive-bui…

bf67b87

…lder into vision

Updated to address feedback

6912f3b

Added language server protocol link

e515cd4

This was referenced Apr 17, 2017

Added #[builder(derive(...)] for #43 #84

Merged

Getters #7

Closed

TedDriggs mentioned this pull request Apr 25, 2017

Declare/forward struct and field-level attributes to builder #90

Closed

TedDriggs closed this May 17, 2018

TedDriggs deleted the vision branch September 4, 2018 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vision proposal #79

Vision proposal #79

TedDriggs commented Apr 14, 2017 •

edited by colin-kiegel

Loading

colin-kiegel left a comment •

edited

Loading

colin-kiegel Apr 15, 2017

TedDriggs Apr 17, 2017

colin-kiegel Apr 15, 2017

TedDriggs Apr 17, 2017

colin-kiegel Apr 15, 2017

colin-kiegel Apr 15, 2017 •

edited

Loading

TedDriggs Apr 17, 2017

faern Apr 19, 2017

colin-kiegel Apr 15, 2017

TedDriggs Apr 17, 2017

colin-kiegel Apr 22, 2017

colin-kiegel Apr 15, 2017

colin-kiegel Apr 15, 2017

colin-kiegel Apr 15, 2017

TedDriggs Apr 17, 2017

colin-kiegel Apr 15, 2017

TedDriggs Apr 17, 2017

colin-kiegel Apr 22, 2017

colin-kiegel Apr 15, 2017

TedDriggs Apr 17, 2017

Vision proposal #79

Vision proposal #79

Conversation

TedDriggs commented Apr 14, 2017 • edited by colin-kiegel Loading

colin-kiegel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-kiegel Apr 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TedDriggs commented Apr 14, 2017 •

edited by colin-kiegel

Loading

colin-kiegel left a comment •

edited

Loading

colin-kiegel Apr 15, 2017 •

edited

Loading