#67 Pre-build validation hook #85

TedDriggs · 2017-04-19T17:05:18Z

This addresses #67 by providing a pre-construction validation hook for structs that use the macro-generated build method. This is consistent with @colin-kiegel and @faern's feedback on wanting a place to perform cross-field validation without manually re-implementing the construction step.

At the moment, this does not handle defaults particularly well. I'm working on a sketch of a plan which exposes get_* methods that return one of:

Ok with the explicit value of that field
Ok with the default value of that field
Err with the uninitialized field message

I suspect this will require some rearchitecting of the struct or of the build method to enable populating those prior to calling validator, but I'm confident it can be done in a backwards-compatible fashion for anyone who hasn't done really strange things in the #[builder(default=...)] expressions (see #65 for examples of things that may cause weirdness).

FYI @colin-kiegel: I've started getting a warning from cargo test that the doctests in lib.rs aren't being run:

WARNING: src/lib.rs -  (line 28) Code block is not currently run as a test, but will in future versions of rustdoc. Please ensure this code block is a runnable test, or use the `ignore` directive.
WARNING: src/lib.rs -  (line 74) Code block is not currently run as a test, but will in future versions of rustdoc. Please ensure this code block is a runnable test, or use the `ignore` directive.

This adds a validation hook when the macro-generated build function is used. The hook borrows the builder and returns a Result<_, String> if any errors are discovered.

faern · 2017-04-19T22:22:17Z

@TedDriggs I thought you said in #67 that you already had a PR for the feature to suppress the build method? Anyway. Would it not be nice to have that as a separate PR since that functionality might be desired and nice to get that merged without being stuck in potential long discussions regarding verification.

TedDriggs · 2017-04-19T22:29:10Z

@faern I do - it's #70. I think having the validator function attribute be part of the build_fn customization makes clearer that the validator doesn't run if the user chooses not to have the macro generate the build method (or whatever the author chooses to name it).

faern · 2017-04-19T22:26:58Z

derive_builder/examples/validator_fn.rs

+
+fn main() {
+    // If we don't set the field `ipsum`,
+    let x = LoremBuilder::default().build().unwrap();


Would it not be nice to set an invalid value and assert the build method returns an Err, for demonstrative purposes?

I do that in the tests; I've been putting happy paths in the examples.

But this main does not demonstrate when or where the validator is called, so I would not say it demonstrates the functionality very explicitly. To me it looks more like a example for the default feature.

I agree, it's too subtle that we are actually testing the else arm of the validate function.

faern · 2017-04-19T22:30:19Z

derive_builder/examples/validator_fn.rs

+extern crate derive_builder;
+
+#[derive(Builder, Debug, PartialEq)]
+#[builder(build_fn(validator="LoremBuilder::validate"))]


Would it not make sense to limit the value of validator to an Ident and force it to be a method on the builder? Then it would reduce duplicating the builder name on this line here. Plus since the first argument passed to the validator function would be the builder it would feel strange to have the validator somewhere else.

I prefer adding features as locked down as possible and expand them if there is demand. Like how Rust and the std lib is kind of conservative before adding stuff. If going the other way it becomes hard to keep backwards compatibility since there might be any combination of strange usages in the wild.

It doesn't have to be a static function on the builder. A free function is completely valid, including a generic free function - a generic function with trait bounds gets really compelling. I've used that before in my serialization code, so I'm deliberately aligning this with how Serde referencing functions in attributes.

You could write validator="Self::validate" to avoid duplicating the method name if you wanted.

Changing this to ident would prevent extending it in the future, as there's no way to differentiate "validate" the struct-local Ident from "validate" the in-scope free function.

Fair enough 👍. I could just not see in what situation it would be usable. If you have a free function with generics and trait bounds, then you would have to implement traits for the builder. I can't say if that is useful, I can't come up with a use case at the moment.

Consider a family of builders which have the common requirement that their server IP address must be organization-internal. It would be easy to implement a short trait with one method to expose the server IP and a validate function with a default implementation.

If we have to name the validate fn, then I agree that it should have the explicit form Self::validate instead of just validate. Alignment with Serde (and also our default="Self::default_foo()", although we require brackets there, which is a different story).

But I could also imagine delegating to a trait

pub trait Validate { fn validate(&self) -> Result<(), String>; }

The use case of @TedDriggs could still be implemented with a generic implementation of Validate for the whole family of builders. The advantage is, that #[builder(build_fn(validate))] would suffice.

The question is however, whether we find a way to bring this Validate trait into scope.

faern · 2017-04-19T22:34:16Z

@TedDriggs Yes, having the validator be an argument to build_fn might be a very nice idea indeed. But it does not stop us from adding build_fn with skip/rename functionality first, separately.

faern · 2017-04-19T22:36:40Z

derive_builder/src/options/mod.rs

@@ -454,7 +459,7 @@ impl<Mode> OptionsBuilder<Mode> where
        trace!("Parsing skip setter `{:?}`", skip);
        self.setter_enabled(!parse_lit_as_bool(skip).unwrap());
    }
-
+    


Can this be removed again?

If you want to review just the build function, please have a look at #70. This builds on that, so we can review and merge that one, and I'll update this to sit on top of any changes we make there.

Or did you mean the trailing whitespace?

Yes, the trailing whitespace :)

faern · 2017-04-19T22:48:44Z

derive_builder/src/options/struct_mode.rs

 use derive_builder_core::{DeprecationNotes, Bindings};

 #[derive(Debug, Clone)]
 pub struct StructMode {
+    build_fn_name: Option<String>,
+    build_fn_enabled: bool,


I don't know very much about this code base yet, so this is more a question than feedback: Could this not be modeled in only one field? If the Option<String> is None that means we don't generate a build method. So whoever creates the defaults for this default to Some("build".to_owned()) instead of None?

It's probably possible - I'm emulating the existing setter code, which handled these attributes separately.

faern · 2017-04-19T22:50:22Z

derive_builder/src/options/struct_mode.rs

@@ -24,7 +27,10 @@ impl OptionsBuilder<StructMode> {
            build_target_vis: ast.vis.clone(),
            builder_name: None,
            builder_vis: None,
+            build_fn_enabled: true,
+            build_fn_name: None,


Not sure which level of nitpicking is usually applied here. But just want to mention that these should be the first fields to maintain the same order as in the struct def.... 😉

This adds a validation hook when the macro-generated build function is used. The hook borrows the builder and returns a Result<_, String> if any errors are discovered.

…e-builder into validate_fn

colin-kiegel

@TedDriggs Thank you very much for your effort. :-)

There is one thing I would like to discuss in particular.

It would be nice, if we could export a Validate trait in derive_builder.

pub trait Validate {
    fn validate(&self) -> Result<(), String>;
}

But proc_macros are not allowed to export any items.
I just tried this suggestion, but it didn't work - not even on nightly.

As a workaround, we could put in derive_builder_core or another helper crate derive_builder_traits, just builder, ... But this would require the user to import two crates. We would not be the first proc_macro crate with this requirement.

Note: There maybe other use cases for derive_builder to export a trait or type definition in the future. There may come a time in the future when we can merge this back into one crate as Rust evolves.

I lean slightly towards the trait. What do you think about it?

colin-kiegel · 2017-04-22T17:41:36Z

derive_builder/CHANGELOG.md

@@ -6,6 +6,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).

 ### Added
 - customize setter names via `#[builder(setter(name="..."))]`
+- customize build_fn name via `#[builder(build_fn(name="..."))]`
+- suppress build method generation via `#[builder(build_fn(skip))]`
+- perform pre-build validation via `#[builder(build_fn(validator="path::to::fn"))]`


I feel like we should name the attribute validate, validation (or validation_fn) instead of validator. Because validator sounds like we are expecting a struct/object.

However it looks like we already have a mix. We are talking about setter /getter (not set function/...), but also build functions (not builder).

Setter/getter are well known concepts and everyone knows they are methods. If we go with build_fn I think validate_fn is the name that is most consistent with that and should be used.

@faern What do you think about the attribute nesting?

should it be nested like build_fn(validate_fn), e.g.
#[builder(build_fn(name="finish", validate_fn="Self::validate"))]

or should it be flat like build_fn, validate_fn, e.g.
#[builder(build_fn(name="finish"), validate_fn="Self::validate")]

Usually we do nesting, to organize settings. But we already have some kind of exception with default which currently only affects the build_fn but is not nested. However default is also a field specific option and may also affect getters in the future. The reason there was that #[builder(build_fn(default))] would have looked a bit weird on a field level. So the situation is definitely a bit different with validate_fn.

I think my current favourite would be validate + nested, e.g. #[builder(build_fn(validate="Self::validate"))]. This way many attributes could be read as imperative verbs, like in please validate this build function via Self::validate, please skip this, please default this, please name this foo, ...

It looks like we have a split, where one person each is in favour of validator, validate_fn or validate. It would be nice to reach a bit more of consensus here. ^^

colin-kiegel · 2017-04-22T17:41:44Z

derive_builder/examples/validator_fn.rs

+extern crate derive_builder;
+
+#[derive(Builder, Debug, PartialEq)]
+#[builder(build_fn(validator="LoremBuilder::validate"))]


If we have to name the validate fn, then I agree that it should have the explicit form Self::validate instead of just validate. Alignment with Serde (and also our default="Self::default_foo()", although we require brackets there, which is a different story).

But I could also imagine delegating to a trait

pub trait Validate { fn validate(&self) -> Result<(), String>; }

The use case of @TedDriggs could still be implemented with a generic implementation of Validate for the whole family of builders. The advantage is, that #[builder(build_fn(validate))] would suffice.

The question is however, whether we find a way to bring this Validate trait into scope.

colin-kiegel · 2017-04-22T17:44:31Z

derive_builder/examples/validator_fn.rs

+
+fn main() {
+    // If we don't set the field `ipsum`,
+    let x = LoremBuilder::default().build().unwrap();


I agree, it's too subtle that we are actually testing the else arm of the validate function.

colin-kiegel · 2017-04-22T17:55:40Z

derive_builder/src/lib.rs

+//! 
+//! The provided function must have the signature `(&FooBuilder) -> Result<_, String>`; 
+//! the `Ok` variant is not used by the `build` method, and must be accessible from the scope
+//! where the target struct is declared. The path does not need to be fully-qualified, and will


The sentence The provided function must have the signature ... and must be accessible ... is to long IMO. Can you split it one way or the other?

Please also add a note somewhere, that default values are not validated, i.e. the validation is run before default values are applied. This may not be obvious.

I missed this and a few other comments; I'll get these tomorrow.

@TedDriggs I already did the missing comments in
TedDriggs@bbd830e. Next time I'll inform you to avoid confusion. :-)

colin-kiegel · 2017-04-22T18:01:46Z

derive_builder/src/lib.rs

+//!
+//! fn main() {
+//!     // If we don't set the field `ipsum`,
+//!     let x = LoremBuilder::default().build().unwrap();


IMO the example should better showcase an invalid input (and not call unwrap, but assert_eq!(... Err(...))).

colin-kiegel · 2017-04-22T18:07:09Z

derive_builder/src/options/struct_options.rs

@@ -5,6 +5,11 @@ use options::DefaultExpression;
 /// These struct options define how the builder is generated.
 #[derive(Debug, Clone)]
 pub struct StructOptions {
+    /// Whether or not this struct should implement its own build method.
+    pub build_fn_enabled: bool,
+


hm, is this new line intentional?

colin-kiegel · 2017-04-22T18:09:47Z

derive_builder/tests/validator_fn.rs

+    // In this case, the validator should have run but passed because there was no value set for `my_effort`.
+    // Adding private `get` methods which return `Result<T, String>` by pulling in defaults will enable the
+    // validation function to be improved here.
+    assert!(&err_msg != "Don't wear yourself out");


please try use assert_ne!(computed, not_expected) instead - the error message will be more helpful. :-)

colin-kiegel · 2017-04-22T18:14:27Z

derive_builder/tests/validator_fn.rs

+#[test]
+fn out_of_bounds() {
+    assert_eq!("Don't wear yourself out", &LoremBuilder::default().my_effort(120).build().unwrap_err());
+    assert_eq!("Your rival is cheating", &LoremBuilder::default().rivals_effort(120).build().unwrap_err());


I prefer to have the field order consistent, i.e. first computed, then expected. That makes it easier to get error messages right. ^^

…e-builder into validate_fn

colin-kiegel · 2017-04-25T15:02:15Z

@TedDriggs @faern I think we are not very far from reaching consensus here. There are only two controversial topics left:

bikeshedding how to name the option (validator, validate, validate_fn)
and the question whether to export and use a Validate trait

Note: I'll start a vacation on sunday, but I still see a chance that we might agree on something before that. ;-)

TedDriggs · 2017-04-25T15:55:58Z

If we ship with the path approach, then we can always add trait support once proc-macro crates are allowed to export more things. It would be identical to how we handle default:

In the word form, e.g. validate, we try to use the trait
In the name=value form, e.g. validate = "Self::validate" we use the specified path

I'm fine with the name being validate; I think that's clear and that name works well for extension to the trait approach in the future.

colin-kiegel · 2017-04-25T16:03:33Z

Ok, cool. That sounds good! :-)

implements pre-build validation hook #85

TedDriggs added 2 commits April 19, 2017 09:52

colin-kiegel#67 - custom validation

254a4f2

This adds a validation hook when the macro-generated build function is used. The hook borrows the builder and returns a Result<_, String> if any errors are discovered.

Merge remote-tracking branch 'upstream/master' into validate_fn

8f74079

Removed expect_err to build on stable

75a3a0f

faern reviewed Apr 19, 2017

View reviewed changes

TedDriggs added 2 commits April 19, 2017 16:48

Stylistic fixes

93e7705

Merge with latest on build_fn

362ddc7

TedDriggs mentioned this pull request Apr 20, 2017

Getters #7

Closed

TedDriggs added 6 commits April 20, 2017 11:21

Resolve merge hiccup with build_fn

a850a0d

colin-kiegel#67 - custom validation

83bb5c8

This adds a validation hook when the macro-generated build function is used. The hook borrows the builder and returns a Result<_, String> if any errors are discovered.

Removed expect_err to build on stable

9a80bfd

Stylistic fixes

01842a5

Resolve merge hiccup with build_fn

308a6ec

Merge branch 'validate_fn' of https://github.com/TedDriggs/rust-deriv…

1f92d84

…e-builder into validate_fn

colin-kiegel reviewed Apr 22, 2017

View reviewed changes

TedDriggs added 3 commits April 24, 2017 11:01

Rebase

524d108

Rebase continues

1cf5ef4

Merge branch 'validate_fn' of https://github.com/TedDriggs/rust-deriv…

35a7872

…e-builder into validate_fn

TedDriggs added 2 commits April 25, 2017 10:53

Merge with master

693b4b0

Rename "validator" to "validate" in attribute

d928dd4

TedDriggs mentioned this pull request Apr 25, 2017

Improved type param bound expressions (#91) #92

Merged

cleanup

bbd830e

colin-kiegel merged commit bbd830e into colin-kiegel:master Apr 26, 2017

colin-kiegel added a commit that referenced this pull request Apr 26, 2017

Merge branch 'validate_fn'

671b7c4

implements pre-build validation hook #85

TedDriggs deleted the validate_fn branch April 26, 2017 17:18

#67 Pre-build validation hook #85

#67 Pre-build validation hook #85

Conversation

TedDriggs commented Apr 19, 2017

faern commented Apr 19, 2017

TedDriggs commented Apr 19, 2017

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

faern commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faern Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-kiegel left a 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 24, 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

colin-kiegel commented Apr 25, 2017 • edited Loading

TedDriggs commented Apr 25, 2017

colin-kiegel commented Apr 25, 2017

faern Apr 19, 2017 •

edited

Loading

colin-kiegel Apr 24, 2017 •

edited

Loading

colin-kiegel commented Apr 25, 2017 •

edited

Loading