From 573f3ae158772b96fc2ef779e0a1ea4dd49d565a Mon Sep 17 00:00:00 2001 From: Colin Kiegel Date: Thu, 13 Apr 2017 12:46:57 +0200 Subject: [PATCH 01/11] refactor workspace - move all integration tests into `derive_builder` crate - shrink `derive_builder_test` crate into `tests/dummy-crate` --- .travis.yml | 18 ++++++++---------- Cargo.toml | 2 +- derive_builder/Cargo.toml | 3 ++- .../tests/compile-fail/deny_empty_default.rs | 0 .../compile-fail/rename_setter_struct_level.rs | 0 .../tests/compiletests.rs | 0 derive_builder/tests/dummy-crate/Cargo.toml | 13 +++++++++++++ .../tests/dummy-crate}/src/lib.rs | 2 ++ .../tests/run-pass/attributes.rs | 0 .../tests/run-pass/custom_types.rs | 0 .../run-pass/deprecate_setter_prefix_syntax.rs | 0 .../run-pass/deprecate_struct-level_default.rs | 0 .../tests/run-pass/empty_struct.rs | 0 .../tests/run-pass/multiple_derives.rs | 0 .../tests/run-pass/no_std.rs | 0 .../tests/try_setter.rs | 0 derive_builder_test/Cargo.toml | 18 ------------------ derive_builder_test/LICENSE-APACHE | 1 - derive_builder_test/LICENSE-MIT | 1 - dev/checkfeatures.sh | 2 +- dev/githook.sh | 5 ++++- dev/nightlytests.sh | 2 +- dev/stylechecks.sh | 2 -- 23 files changed, 32 insertions(+), 37 deletions(-) rename {derive_builder_test => derive_builder}/tests/compile-fail/deny_empty_default.rs (100%) rename {derive_builder_test => derive_builder}/tests/compile-fail/rename_setter_struct_level.rs (100%) rename {derive_builder_test => derive_builder}/tests/compiletests.rs (100%) create mode 100644 derive_builder/tests/dummy-crate/Cargo.toml rename {derive_builder_test => derive_builder/tests/dummy-crate}/src/lib.rs (83%) rename {derive_builder_test => derive_builder}/tests/run-pass/attributes.rs (100%) rename {derive_builder_test => derive_builder}/tests/run-pass/custom_types.rs (100%) rename {derive_builder_test => derive_builder}/tests/run-pass/deprecate_setter_prefix_syntax.rs (100%) rename {derive_builder_test => derive_builder}/tests/run-pass/deprecate_struct-level_default.rs (100%) rename {derive_builder_test => derive_builder}/tests/run-pass/empty_struct.rs (100%) rename {derive_builder_test => derive_builder}/tests/run-pass/multiple_derives.rs (100%) rename {derive_builder_test => derive_builder}/tests/run-pass/no_std.rs (100%) rename {derive_builder_test => derive_builder}/tests/try_setter.rs (100%) delete mode 100644 derive_builder_test/Cargo.toml delete mode 120000 derive_builder_test/LICENSE-APACHE delete mode 120000 derive_builder_test/LICENSE-MIT diff --git a/.travis.yml b/.travis.yml index 999b15aa..6b3c2339 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,9 +8,9 @@ rust: matrix: include: - rust: 1.15.0 - env: JOB=build CARGO_FEATURES="struct_default" + env: JOB=build CARGO_FEATURES="skeptic_tests struct_default" - rust: nightly - env: JOB=build CARGO_FEATURES="nightlytests logging" + env: JOB=build CARGO_FEATURES="skeptic_tests nightlytests logging" - rust: nightly env: JOB=style_check allow_failures: @@ -20,7 +20,7 @@ env: matrix: - JOB=build global: - - CARGO_FEATURES="" + - CARGO_FEATURES="skeptic_tests" - RUST_BACKTRACE=1 - TRAVIS_CARGO_NIGHTLY_FEATURE="" - PKGNAME=derive_builder @@ -50,22 +50,20 @@ script: | # We have to consider the following limitations of cargo in rustc 1.15: # - no support for virtual worskpaces, instead we have to cd into a crate. # - cargo build/doc does not support `--all`, luckily the - # `derive_builder_test` crate will implicitly build/doc the - # `derive_builder` and `derive_builder_core` crates too. + # `derive_builder` crate will implicitly build/doc the + # `derive_builder_core` crate too. commands=( - "cd derive_builder_test && travis-cargo build -- --features \"$CARGO_FEATURES\"" - "cd derive_builder_test && travis-cargo test -- --all --no-fail-fast --features \"$CARGO_FEATURES\"" - "cd derive_builder_test && travis-cargo doc" + "cd derive_builder && travis-cargo build -- --features \"$CARGO_FEATURES\"" + "cd derive_builder && travis-cargo test -- --all --no-fail-fast --features \"$CARGO_FEATURES\"" + "cd derive_builder && travis-cargo doc" ) ;; style_check) commands=( "cd derive_builder_core && cargo clippy -- -Dclippy" "cd derive_builder && cargo clippy -- -Dclippy" - "cd derive_builder_test && cargo clippy -- -Dclippy" "cd derive_builder_core && cargo fmt -- --write-mode diff" "cd derive_builder && cargo fmt -- --write-mode diff" - "cd derive_builder_test && cargo fmt -- --write-mode diff" ) ;; *) diff --git a/Cargo.toml b/Cargo.toml index e4548916..2a676ea1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,2 +1,2 @@ [workspace] -members = ["derive_builder", "derive_builder_core", "derive_builder_test"] +members = ["derive_builder", "derive_builder_core"] diff --git a/derive_builder/Cargo.toml b/derive_builder/Cargo.toml index 345988af..dfa67288 100644 --- a/derive_builder/Cargo.toml +++ b/derive_builder/Cargo.toml @@ -25,7 +25,7 @@ proc-macro = true logging = [ "log", "env_logger", "derive_builder_core/logging" ] struct_default = [] skeptic_tests = ["skeptic"] -nightlytests = [] +nightlytests = ["compiletest_rs"] [dependencies] syn = "0.11" @@ -34,6 +34,7 @@ log = { version = "0.3", optional = true } env_logger = { version = "0.4", optional = true } derive_builder_core = { version = "0.1", path = "../derive_builder_core" } skeptic = { version = "0.9", optional = true } +compiletest_rs = { version = "0.2", optional = true } [build-dependencies] skeptic = { version = "0.9", optional = true } diff --git a/derive_builder_test/tests/compile-fail/deny_empty_default.rs b/derive_builder/tests/compile-fail/deny_empty_default.rs similarity index 100% rename from derive_builder_test/tests/compile-fail/deny_empty_default.rs rename to derive_builder/tests/compile-fail/deny_empty_default.rs diff --git a/derive_builder_test/tests/compile-fail/rename_setter_struct_level.rs b/derive_builder/tests/compile-fail/rename_setter_struct_level.rs similarity index 100% rename from derive_builder_test/tests/compile-fail/rename_setter_struct_level.rs rename to derive_builder/tests/compile-fail/rename_setter_struct_level.rs diff --git a/derive_builder_test/tests/compiletests.rs b/derive_builder/tests/compiletests.rs similarity index 100% rename from derive_builder_test/tests/compiletests.rs rename to derive_builder/tests/compiletests.rs diff --git a/derive_builder/tests/dummy-crate/Cargo.toml b/derive_builder/tests/dummy-crate/Cargo.toml new file mode 100644 index 00000000..e1baf731 --- /dev/null +++ b/derive_builder/tests/dummy-crate/Cargo.toml @@ -0,0 +1,13 @@ +[package] +name = "derive_builder_dummy_crate" +version = "0.1.0" +authors = ["Colin Kiegel "] +description = "This dummy crate serves as a playground to **manually** inspect things like generated documentation. It is **not** tested automatically." +license = "MIT/Apache-2.0" +publish = false + +[lib] +path = "src/lib.rs" + +[dependencies] +derive_builder = { path = "../../" } diff --git a/derive_builder_test/src/lib.rs b/derive_builder/tests/dummy-crate/src/lib.rs similarity index 83% rename from derive_builder_test/src/lib.rs rename to derive_builder/tests/dummy-crate/src/lib.rs index 8290e8b2..2e9bedb8 100644 --- a/derive_builder_test/src/lib.rs +++ b/derive_builder/tests/dummy-crate/src/lib.rs @@ -1,3 +1,5 @@ +/// This dummy crate serves as a playground to inspect things like generated documentation + #[macro_use] extern crate derive_builder; diff --git a/derive_builder_test/tests/run-pass/attributes.rs b/derive_builder/tests/run-pass/attributes.rs similarity index 100% rename from derive_builder_test/tests/run-pass/attributes.rs rename to derive_builder/tests/run-pass/attributes.rs diff --git a/derive_builder_test/tests/run-pass/custom_types.rs b/derive_builder/tests/run-pass/custom_types.rs similarity index 100% rename from derive_builder_test/tests/run-pass/custom_types.rs rename to derive_builder/tests/run-pass/custom_types.rs diff --git a/derive_builder_test/tests/run-pass/deprecate_setter_prefix_syntax.rs b/derive_builder/tests/run-pass/deprecate_setter_prefix_syntax.rs similarity index 100% rename from derive_builder_test/tests/run-pass/deprecate_setter_prefix_syntax.rs rename to derive_builder/tests/run-pass/deprecate_setter_prefix_syntax.rs diff --git a/derive_builder_test/tests/run-pass/deprecate_struct-level_default.rs b/derive_builder/tests/run-pass/deprecate_struct-level_default.rs similarity index 100% rename from derive_builder_test/tests/run-pass/deprecate_struct-level_default.rs rename to derive_builder/tests/run-pass/deprecate_struct-level_default.rs diff --git a/derive_builder_test/tests/run-pass/empty_struct.rs b/derive_builder/tests/run-pass/empty_struct.rs similarity index 100% rename from derive_builder_test/tests/run-pass/empty_struct.rs rename to derive_builder/tests/run-pass/empty_struct.rs diff --git a/derive_builder_test/tests/run-pass/multiple_derives.rs b/derive_builder/tests/run-pass/multiple_derives.rs similarity index 100% rename from derive_builder_test/tests/run-pass/multiple_derives.rs rename to derive_builder/tests/run-pass/multiple_derives.rs diff --git a/derive_builder_test/tests/run-pass/no_std.rs b/derive_builder/tests/run-pass/no_std.rs similarity index 100% rename from derive_builder_test/tests/run-pass/no_std.rs rename to derive_builder/tests/run-pass/no_std.rs diff --git a/derive_builder_test/tests/try_setter.rs b/derive_builder/tests/try_setter.rs similarity index 100% rename from derive_builder_test/tests/try_setter.rs rename to derive_builder/tests/try_setter.rs diff --git a/derive_builder_test/Cargo.toml b/derive_builder_test/Cargo.toml deleted file mode 100644 index 71928b41..00000000 --- a/derive_builder_test/Cargo.toml +++ /dev/null @@ -1,18 +0,0 @@ -[package] -name = "derive_builder_test" -version = "0.1.0" -authors = ["Colin Kiegel "] -license = "MIT/Apache-2.0" -publish = false - -[lib] -path = "src/lib.rs" - -[features] -nightlytests = ["compiletest_rs", "derive_builder/nightlytests"] -logging = [ "derive_builder/logging" ] -struct_default = [ "derive_builder/struct_default" ] - -[dependencies] -derive_builder = { path = "../derive_builder", features = ["skeptic_tests"] } -compiletest_rs = { version = "0.2", optional = true } diff --git a/derive_builder_test/LICENSE-APACHE b/derive_builder_test/LICENSE-APACHE deleted file mode 120000 index 965b606f..00000000 --- a/derive_builder_test/LICENSE-APACHE +++ /dev/null @@ -1 +0,0 @@ -../LICENSE-APACHE \ No newline at end of file diff --git a/derive_builder_test/LICENSE-MIT b/derive_builder_test/LICENSE-MIT deleted file mode 120000 index 76219eb7..00000000 --- a/derive_builder_test/LICENSE-MIT +++ /dev/null @@ -1 +0,0 @@ -../LICENSE-MIT \ No newline at end of file diff --git a/dev/checkfeatures.sh b/dev/checkfeatures.sh index b70c407b..e370fa8d 100755 --- a/dev/checkfeatures.sh +++ b/dev/checkfeatures.sh @@ -4,7 +4,7 @@ function main { export CARGO_TARGET_DIR="../target/__checkfeatures" commands=( - "cd derive_builder_test && rustup run 1.15.0 cargo test --all --color always --features struct_default" + "cd derive_builder && rustup run 1.15.0 cargo test --all --color always --features \"skeptic_tests struct_default\"" ) dev/travis-run-all.sh "${commands[@]}" diff --git a/dev/githook.sh b/dev/githook.sh index 660139fe..566bd06f 100755 --- a/dev/githook.sh +++ b/dev/githook.sh @@ -61,7 +61,10 @@ function run_tests_on { echo_begin "Running tests on $1" result=$( exec 2>&1 - ([ "$rustup" == true ] && rustup update $1) && (cd derive_builder_test && rustup run "$1" cargo test --all --color always) + if [ "$rustup" == true ]; then + rustup update $1 || exit + fi + cd derive_builder && rustup run "$1" cargo test --all --color always --features skeptic_tests ); ret=$? check_or_echo $ret "" "$result" } diff --git a/dev/nightlytests.sh b/dev/nightlytests.sh index d97cc83e..fb8f78b2 100755 --- a/dev/nightlytests.sh +++ b/dev/nightlytests.sh @@ -4,7 +4,7 @@ function main { export CARGO_TARGET_DIR="../target/__nightlytests" commands=( - "cd derive_builder_test && rustup run nightly cargo test --all --color always --features nightlytests" + "cd derive_builder && rustup run nightly cargo test --all --color always --features \"skeptic_tests nightlytests\"" ) dev/travis-run-all.sh "${commands[@]}" diff --git a/dev/stylechecks.sh b/dev/stylechecks.sh index 7a160b14..550f3562 100755 --- a/dev/stylechecks.sh +++ b/dev/stylechecks.sh @@ -6,10 +6,8 @@ function main { commands=( "cd derive_builder_core && cargo clippy -- -Dclippy --color always" "cd derive_builder && cargo clippy -- -Dclippy --color always" - "cd derive_builder_test && cargo clippy -- -Dclippy --color always" "cd derive_builder_core && cargo fmt -- --write-mode diff" "cd derive_builder && cargo fmt -- --write-mode diff" - "cd derive_builder_test && cargo fmt -- --write-mode diff" ) dev/travis-run-all.sh "${commands[@]}" From 12941f599f3f0d4d277341f358b53443160d69ff Mon Sep 17 00:00:00 2001 From: Colin Kiegel Date: Thu, 13 Apr 2017 12:58:30 +0200 Subject: [PATCH 02/11] fix changelog --- derive_builder/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder/CHANGELOG.md b/derive_builder/CHANGELOG.md index 63e6ba76..29c44972 100644 --- a/derive_builder/CHANGELOG.md +++ b/derive_builder/CHANGELOG.md @@ -106,7 +106,7 @@ Requires Rust 1.15 or newer. - generate setter methods - support for generic structs -[Unreleased]: https://github.com/colin-kiegel/rust-derive-builder/compare/v0.4.0...HEAD +[Unreleased]: https://github.com/colin-kiegel/rust-derive-builder/compare/v0.4.4...HEAD [0.4.4]: https://github.com/colin-kiegel/rust-derive-builder/compare/v0.4.3...v0.4.4 [0.4.3]: https://github.com/colin-kiegel/rust-derive-builder/compare/v0.4.2...v0.4.3 [0.4.2]: https://github.com/colin-kiegel/rust-derive-builder/compare/v0.4.1...v0.4.2 From d13d04a7b31e01dee7ea3ee23e3810d722c62d7d Mon Sep 17 00:00:00 2001 From: Colin Kiegel Date: Thu, 13 Apr 2017 13:13:45 +0200 Subject: [PATCH 03/11] CONTRIBUTING.md --- CONTRIBUTING.md | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 CONTRIBUTING.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 00000000..b6b317a7 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,30 @@ +# Contributing to `derive_builder` + +Thank you for your interest in contributing to this crate! + +## Pull Requests + +Please make [pull requests] against the `master` branch. +[pull requests]: https://help.github.com/articles/using-pull-requests/ + +All pull requests should clearly describe what they intend to do, or link to +a github issue, where this is explained. + +You should try to make sure your pull request passes all tests. Since some +tests are behind feature gates it's best to run the script `dev/githook.sh` as +described in `dev/README.md`. This script is intended to be primarily used as a +pre-push git hook, but it can be called manually as well. + +Please follow this checklist +- update the `CHANGELOG.md` - add a section `[Unreleased]` at the top, if + that's missing. +- update the documentation in `lib.rs` (optional: `README.md`) +- add unit tests to `derive_builder_core`, if appropriate +- add integration tests to `derive_builder` with different variants and also + try to test possible side effects with other features of this crate. + +### Early Feedback and Help + +If you're stuck somewhere and need help or if you just want some early feedback, +you can either open an issue, or send a preliminary PR. In that case, please +mark it as **work in progress (WIP)** and state your questions. From cbeb0ed539982dd528e7cde85506d796556e4119 Mon Sep 17 00:00:00 2001 From: Ted Driggs Date: Thu, 13 Apr 2017 14:08:13 -0700 Subject: [PATCH 04/11] Added doc comment explaining how OptionsBuilder gets used. --- derive_builder/src/doc_tpl/builder_struct.md | 2 +- derive_builder/src/options/mod.rs | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/derive_builder/src/doc_tpl/builder_struct.md b/derive_builder/src/doc_tpl/builder_struct.md index adb1ecef..6a9b2185 100644 --- a/derive_builder/src/doc_tpl/builder_struct.md +++ b/derive_builder/src/doc_tpl/builder_struct.md @@ -1 +1 @@ -Builder for `{struct_name}`. +Builder for [`{struct_name}`](struct.{struct_name}.html). diff --git a/derive_builder/src/options/mod.rs b/derive_builder/src/options/mod.rs index 9584e2da..c18966dd 100644 --- a/derive_builder/src/options/mod.rs +++ b/derive_builder/src/options/mod.rs @@ -1,3 +1,16 @@ +//! Types and functions for parsing attribute options. +//! +//! Attribute parsing occurs in two stages: +//! +//! 1. Builder options on the struct are parsed into `OptionsBuilder`. +//! 1. The `OptionsBuilder` instance is converted into a starting point for the +//! per-field options (`OptionsBuilder`) and the finished struct-level config, +//! called `StructOptions`. +//! 1. Each struct field is parsed, with discovered attributes overriding or augmenting the +//! options specified at the struct level. This creates one `OptionsBuilder` per +//! struct field on the input/target type. Once complete, these get converted into +//! `FieldOptions` instances. + use syn; use derive_builder_core::BuilderPattern; From d55c8ff2a232df6e8c2ec7b0a4f5ce4807992df5 Mon Sep 17 00:00:00 2001 From: Ted Driggs Date: Fri, 14 Apr 2017 10:27:23 -0700 Subject: [PATCH 05/11] Vision proposal --- vision_doc.md | 279 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 279 insertions(+) create mode 100644 vision_doc.md diff --git a/vision_doc.md b/vision_doc.md new file mode 100644 index 00000000..4bd36b4a --- /dev/null +++ b/vision_doc.md @@ -0,0 +1,279 @@ +# Vision +`derive_builder` provides an ergonomic way to deal with construction of guaranteed-complete objects from a range of inputs: + +* Statically-known values +* 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. + +## 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. + +## 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). + +## 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. + +In this proposal, an infallible builder is one which declares `#[builder(default)]` at the struct level and does not skip the automatic build function, which requires that all fields be optional and any well-formed value be acceptable. When a builder meets these criteria, a `From<&mut TargetBuilder> for Target` impl would be automatically emitted (exact implementations emitted would depend on builder pattern). **Open question: does this change the signature of the emitted `build` method, add a new inherent method, or just add the `Into` implementation?** + +To handle the mix-required-and-optional case, exporting crate would refactor to have a `Target` struct and a `TargetOptions` struct, with a builder derived on one or both of those. The `TargetOptions` struct would have only optional fields, thus enabling an infallible build of it. + +# Use Case +Consider a `ConnectionPool` object. It has: + +* A set of required fields, such as `host`, `api_key`, etc. +* A set of optional/defaulted fields: `timeout`, `retry_policy`, etc. +* A private TCP connection that is opened in `ConnectionPool::new` and closed during drop + +This connection pool object's data is pulled from multiple sources, in decreasing priority: + +1. Command-line args +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. + +Under this vision proposal, `ConnectionPool` would be declared as follows: + +```rust + +#[derive(Debug, Builder, Serialize)] +#[builder(name = "ConnectionInfo", + setter(into), + try_setter, + build_fn(skip), + derive(Deserialize), + preserve_attrs(serde))] +pub struct ConnectionPool { + host: String, + + api_key: ApiKey, + + #[builder(default="ConnectionPool::default_timeout()")] + timeout: Duration, + + #[builder(setter(skip))] + #[serde(skip_serializing)] + socket: Socket +} + +impl ConnectionPool { + /// Could be private if the builder was meant to be only way to create instance. + pub fn new(host: String, api_key: ApiKey, timeout: Duration) -> Result { + // @FAERN; validation would occur here + + let socket = unimplemented!(); // try to open the socket... + ConnectionPool { + host: host, + api_key: api_key, + timeout: timeout + } + } + + fn default_timeout() -> Duration { + Duration::from_millis(5000) + } +} + +impl ConnectionInfo { + /// Establish a connection using the given information. + pub fn connect(&self) -> Result { + ConnectionPool::new( + self.get_host()?, + self.get_api_key()?, + self.get_timeout()?) + ) + } +} +``` + +# Example +```rust +mod exporting_crate { + use std::net::IpAddr; + use serde::{Serialize, Deserialize}; + + #[derive(Debug, Clone, Copy, Serialize, Deserialize)] + #[serde(rename_all = "snake_case")] + pub enum Cycle { + Auto, + Slow, + Medium, + Fast, + } + + impl FromStr for Cycle { + type Err = String; + // ... impl elided + } + + #[derive(Debug, Clone, Serialize, Deserialize)] + #[serde(tag = "type", rename_all = "snake_case")] + pub enum Key { + Str(String), + IpAddr(IpAddr), + } + + #[derive(Debug, Clone)] + pub enum Filter { + Approx(String), + Exact(Key), + } + + impl Serialize for Filter { + // ... impl elided + } + + impl Deserialize for Filter { + // ... impl elided + } + + /// A data request for the Northwind metrics API. + /// This type is focused on modeling a well-formed stat + /// request, it doesn't allow for partially-built states. + /// + /// This invents a few new properties for the builder macro: + /// + /// * `build_fn(skip)`: This tells the builder not to auto-generate the + /// build method. + /// * `preserve_attributes`: This tells the builder to include all #[serde] + /// attributes on the `StatRequestBuilder` fields. + /// * `target_factory`: Name needs work, but this tells the macro to add an + /// 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 { + /// This particular field should be named `stat` on the inbound request; + /// that requires a field-level annotation. + #[serde(rename(deserialize = "stat"))] + stat_name: String, + + cycle: Cycle, + + #[serde(default, rename = "filter", skip_serializing_if="Option::is_none")] + #[builder(default)] + key_filter: Option, + } + + impl StatRequest { + pub fn new(stat_name: String, cycle: Cycle) -> Result { + StatRequest::with_filter(stat_name, cycle, None) + } + + /// Creates a new `StatRequest` with the specified filter (`None` is allowed). + /// + /// A filter is required for detail stats for perf reasons; this method will + /// return an error if one is needed and `None` is provided, or if one is provided + /// and the specified stat does not support filtering. + pub fn with_filter>>(stat_name: String, + cycle: Cycle, + filter: F) + -> Result { + let requires_filter = stat_name.ends_with("_detail"); + let filter_opt = filter.into(); + match (requires_filter, filter_opt.is_some()) { + (true, false) => Err(format!("Stat `{}` requires a filter", stat_name)), + (false, true) => Err(format!("Stat `{}` is not a detail metric")), + _ => { + Ok(StatRequest { + stat_name: stat_name, + key_filter: filter_opt, + cycle: cycle, + }) + } + } + } + } + + impl StatRequestBuilder { + /// Create a stat request by calling `StatRequest::with_filter`. This method + /// will return an error if a required field is not initialized, or if the + /// values fail validation. + pub fn build(&self) -> Result { + // These getters are private methods which return Result; + // the error arises if the field is uninitialized and has no default. + StatRequest::with_filter( + self.get_stat_name()?, + self.get_cycle()?, + self.get_filter()?) + } + } + + impl TryFrom for StatRequest { + type Error = String; + + fn try_from(v: StatRequestBuilder) -> Result { + v.build() + } + } +} + +mod consuming_crate { + mod errors { + error_chain! { + // ... errors elided + } + } + + use errors::{Error, ChainedError, Result, ResultExt}; + use exporting_crate::{Cycle, Key, Filter, StatRequest}; + + fn merge_args(inbound_req: &str, cycle_lock: Option) -> Result { + // deserialize from the inbound request into a builder; this enables further + // validation and can be used to generate nicer failures if required fields are + // missing. This is using error_chain and the carrier operator's conversion + // feature to be very brief while still returning a strongly-typed error specific + // to the consuming crate. + let mut builder = serde_json::from_str(req_body)?; + + // apply the cycle override, if one was provided. + if let Some(cl) = cycle_lock { + builder.cycle(cl); + } + + builder.build() + } + + fn request(query: &StatRequest) -> Result<()> { + let rsp = reqwest::Client::new()? + .post("https://northwind.com/api/v1/metrics") + .json(&query) + .send()?; + + rsp.validate_status() + .map(|| ()) + .chain_err(|| "The server returned an error") + } + + /// Run an arbitrary stat request. + pub fn run(req: &str) -> Result<()> { + // Look for an environment-defined cycle lock and parse it if found. + let cycle_lock = dotenv::var("CYCLE_LOCK").map(Cycle::from_str)?; + + // create the StatRequest; still relying on the error_chain + carrier pair + // seen above. Now, `def` is a `StatRequest` which is ready to use. + let query = merge_args(req, cycle_lock)?; + + request(query) + } + + /// Test if the service is up or down. + pub fn health_check(cycle: Cycle) -> Result<()> { + // this gets a fresh StatRequestBuilder, despite not having + // imported the + let query = StatRequest::builder() + .stat_name("system.is_up") + .cycle(cycle) + .build() + .chain_err(|| "Health check is malformed")?; + + request(query); + } +} +``` \ No newline at end of file From cd8e2951c6b62b17575f76bae86b69cd62366647 Mon Sep 17 00:00:00 2001 From: Colin Kiegel Date: Sat, 15 Apr 2017 12:40:37 +0200 Subject: [PATCH 06/11] cleanup and rustfmt --- derive_builder/examples/custom_defaults.rs | 17 ++- derive_builder/examples/deny_missing_docs.rs | 5 +- derive_builder/src/options/mod.rs | 6 +- derive_builder/tests/compiletests.rs | 3 +- derive_builder/tests/custom_default.rs | 67 ++++----- derive_builder/tests/generic_structs.rs | 5 +- derive_builder/tests/setter_into.rs | 20 +-- derive_builder/tests/setter_name.rs | 2 +- derive_builder/tests/setter_visibility.rs | 10 +- derive_builder/tests/skip-setter.rs | 29 ++-- derive_builder/tests/try_setter.rs | 5 +- derive_builder_core/src/bindings.rs | 137 ++++++++----------- derive_builder_core/src/build_method.rs | 6 +- derive_builder_core/src/builder.rs | 5 +- derive_builder_core/src/initializer.rs | 27 ++-- derive_builder_core/src/setter.rs | 16 +-- derive_builder_core/src/tokens.rs | 2 +- 17 files changed, 174 insertions(+), 188 deletions(-) diff --git a/derive_builder/examples/custom_defaults.rs b/derive_builder/examples/custom_defaults.rs index 8a3f55c5..5d41179e 100644 --- a/derive_builder/examples/custom_defaults.rs +++ b/derive_builder/examples/custom_defaults.rs @@ -14,7 +14,9 @@ struct Lorem { impl LoremBuilder { fn default_dolor(&self) -> Result { - self.ipsum.clone().ok_or_else(|| "ipsum must be initialized to build dolor".to_string()) + self.ipsum + .clone() + .ok_or_else(|| "ipsum must be initialized to build dolor".to_string()) } fn default_sit(&self) -> Result { @@ -39,10 +41,11 @@ fn main() { .build() .unwrap(); - assert_eq!(x, Lorem { - ipsum: "ipsum".to_string(), - dolor: "ipsum".to_string(), - sit: "sit ipsum".to_string(), - amet: "amet ipsum".to_string(), - }); + assert_eq!(x, + Lorem { + ipsum: "ipsum".to_string(), + dolor: "ipsum".to_string(), + sit: "sit ipsum".to_string(), + amet: "amet ipsum".to_string(), + }); } diff --git a/derive_builder/examples/deny_missing_docs.rs b/derive_builder/examples/deny_missing_docs.rs index e5938ff8..1121a1ec 100644 --- a/derive_builder/examples/deny_missing_docs.rs +++ b/derive_builder/examples/deny_missing_docs.rs @@ -15,6 +15,9 @@ pub struct Letter { } fn main() { - let x = LetterBuilder::default().message("Hello World!").build().unwrap(); + let x = LetterBuilder::default() + .message("Hello World!") + .build() + .unwrap(); println!("{}", x.message); } diff --git a/derive_builder/src/options/mod.rs b/derive_builder/src/options/mod.rs index c18966dd..d68a0325 100644 --- a/derive_builder/src/options/mod.rs +++ b/derive_builder/src/options/mod.rs @@ -1,12 +1,12 @@ //! Types and functions for parsing attribute options. //! -//! Attribute parsing occurs in two stages: +//! Attribute parsing occurs in multiple stages: //! //! 1. Builder options on the struct are parsed into `OptionsBuilder`. -//! 1. The `OptionsBuilder` instance is converted into a starting point for the +//! 1. The `OptionsBuilder` instance is converted into a starting point for the //! per-field options (`OptionsBuilder`) and the finished struct-level config, //! called `StructOptions`. -//! 1. Each struct field is parsed, with discovered attributes overriding or augmenting the +//! 1. Each struct field is parsed, with discovered attributes overriding or augmenting the //! options specified at the struct level. This creates one `OptionsBuilder` per //! struct field on the input/target type. Once complete, these get converted into //! `FieldOptions` instances. diff --git a/derive_builder/tests/compiletests.rs b/derive_builder/tests/compiletests.rs index 022793cd..32199fca 100644 --- a/derive_builder/tests/compiletests.rs +++ b/derive_builder/tests/compiletests.rs @@ -32,8 +32,7 @@ fn run_mode(mode: &'static str) { let build_dir = env::var("CARGO_TARGET_DIR").unwrap_or(format!("{}/target", base_dir)); let artefacts_dir = format!("{}/{}", build_dir, PROFILE); - config.target_rustcflags = - Some(format!("-L {} -L {}/deps", artefacts_dir, artefacts_dir)); + config.target_rustcflags = Some(format!("-L {} -L {}/deps", artefacts_dir, artefacts_dir)); compiletest::run_tests(&config); } diff --git a/derive_builder/tests/custom_default.rs b/derive_builder/tests/custom_default.rs index 2aa5e416..e440b67e 100644 --- a/derive_builder/tests/custom_default.rs +++ b/derive_builder/tests/custom_default.rs @@ -36,13 +36,14 @@ mod field_level { .build() .unwrap(); - assert_eq!(x, Lorem { - required: "ipsum".to_string(), - explicit_default: "".to_string(), - escaped_default: "foo".to_string(), - raw_default: "Hello World!".to_string(), - computed_default: "ipsum-EMPTY-EMPTY-EMPTY".to_string(), - }); + assert_eq!(x, + Lorem { + required: "ipsum".to_string(), + explicit_default: "".to_string(), + escaped_default: "foo".to_string(), + raw_default: "Hello World!".to_string(), + computed_default: "ipsum-EMPTY-EMPTY-EMPTY".to_string(), + }); } #[test] @@ -55,13 +56,14 @@ mod field_level { .build() .unwrap(); - assert_eq!(x, Lorem { - required: "ipsum".to_string(), - explicit_default: "lorem".to_string(), - escaped_default: "dolor".to_string(), - raw_default: "sit".to_string(), - computed_default: "ipsum-lorem-dolor-sit".to_string(), - }); + assert_eq!(x, + Lorem { + required: "ipsum".to_string(), + explicit_default: "lorem".to_string(), + escaped_default: "dolor".to_string(), + raw_default: "sit".to_string(), + computed_default: "ipsum-lorem-dolor-sit".to_string(), + }); } } @@ -107,30 +109,28 @@ mod struct_level { #[test] fn explicit_defaults_are_equal() { - let lorem = LoremBuilder::default() - .build() - .unwrap(); + let lorem = LoremBuilder::default().build().unwrap(); // new behaviour starting with 0.5.x: #[cfg(feature = "struct_default")] - assert_eq!(lorem, Lorem { - overwritten: true, - ..explicit_default() - }); + assert_eq!(lorem, + Lorem { + overwritten: true, + ..explicit_default() + }); // old behaviour since 0.4.x: #[cfg(not(feature = "struct_default"))] - assert_eq!(lorem, Lorem { - overwritten: true, - not_type_default: explicit_default(), - }); + assert_eq!(lorem, + Lorem { + overwritten: true, + not_type_default: explicit_default(), + }); } #[test] fn implicit_defaults_are_equal() { - let ipsum = IpsumBuilder::default() - .build() - .unwrap(); + let ipsum = IpsumBuilder::default().build().unwrap(); // new behaviour starting with 0.5.x: #[cfg(feature = "struct_default")] @@ -138,11 +138,12 @@ mod struct_level { // old behaviour since 0.4.x: #[cfg(not(feature = "struct_default"))] - assert_eq!(ipsum, Ipsum { - not_type_default: Default::default(), - also_custom: Default::default(), - is_type_default: Default::default(), - }); + assert_eq!(ipsum, + Ipsum { + not_type_default: Default::default(), + also_custom: Default::default(), + is_type_default: Default::default(), + }); } #[test] diff --git a/derive_builder/tests/generic_structs.rs b/derive_builder/tests/generic_structs.rs index f3e6c386..025a608b 100644 --- a/derive_builder/tests/generic_structs.rs +++ b/derive_builder/tests/generic_structs.rs @@ -51,8 +51,5 @@ fn generic_reference_builder() { .build() .unwrap(); - assert_eq!(x, - GenericReference { - bar: Some(&BAR), - }); + assert_eq!(x, GenericReference { bar: Some(&BAR) }); } diff --git a/derive_builder/tests/setter_into.rs b/derive_builder/tests/setter_into.rs index 07a5de80..ab1411c0 100644 --- a/derive_builder/tests/setter_into.rs +++ b/derive_builder/tests/setter_into.rs @@ -17,26 +17,14 @@ struct Ipsum { #[test] fn generic_field() { - let x = LoremBuilder::default() - .foo("foo") - .build() - .unwrap(); + let x = LoremBuilder::default().foo("foo").build().unwrap(); - assert_eq!(x, - Lorem { - foo: "foo".to_string(), - }); + assert_eq!(x, Lorem { foo: "foo".to_string() }); } #[test] fn generic_struct() { - let x = IpsumBuilder::default() - .foo(42u8) - .build() - .unwrap(); + let x = IpsumBuilder::default().foo(42u8).build().unwrap(); - assert_eq!(x, - Ipsum { - foo: 42u32, - }); + assert_eq!(x, Ipsum { foo: 42u32 }); } diff --git a/derive_builder/tests/setter_name.rs b/derive_builder/tests/setter_name.rs index fc0f95a1..8c22bfd0 100644 --- a/derive_builder/tests/setter_name.rs +++ b/derive_builder/tests/setter_name.rs @@ -7,7 +7,7 @@ extern crate derive_builder; #[builder(setter(prefix="with"))] struct Lorem { ipsum: &'static str, - #[builder(setter(name="foo"))] // takes precedence over prefix + #[builder(setter(name="foo"))] pub dolor: &'static str, } diff --git a/derive_builder/tests/setter_visibility.rs b/derive_builder/tests/setter_visibility.rs index a0e369da..92ff2cad 100644 --- a/derive_builder/tests/setter_visibility.rs +++ b/derive_builder/tests/setter_visibility.rs @@ -51,7 +51,10 @@ pub mod foo { #[test] #[should_panic(expected="`private` must be initialized")] fn public_setters_override_foreign_module() { - let x = foo::LoremBuilder::default().public("Hello").build().unwrap(); + let x = foo::LoremBuilder::default() + .public("Hello") + .build() + .unwrap(); assert_eq!(x.public, String::from("Hello")); } @@ -59,7 +62,10 @@ fn public_setters_override_foreign_module() { #[test] #[should_panic(expected="`private` must be initialized")] fn public_setters_foreign_module() { - let y = foo::IpsumBuilder::default().public("Hello").build().unwrap(); + let y = foo::IpsumBuilder::default() + .public("Hello") + .build() + .unwrap(); assert_eq!(y.public, String::from("Hello")); } diff --git a/derive_builder/tests/skip-setter.rs b/derive_builder/tests/skip-setter.rs index f8c78c8d..75fd2952 100644 --- a/derive_builder/tests/skip-setter.rs +++ b/derive_builder/tests/skip-setter.rs @@ -48,9 +48,9 @@ struct SetterOptInStructDefault { struct SetterOptInFieldDefault { #[builder(setter(skip), default = "new_notdefaultable()")] setter_skipped_with_field_default: NotDefaultable, - + #[builder(default)] - setter_present_by_default: u32 + setter_present_by_default: u32, } // compile test @@ -81,8 +81,10 @@ impl Default for SetterOptInStructDefault { #[test] fn setter_opt_out() { - let x: SetterOptOut = - SetterOptOutBuilder::default().setter_present_by_explicit_default(42u32).build().unwrap(); + let x: SetterOptOut = SetterOptOutBuilder::default() + .setter_present_by_explicit_default(42u32) + .build() + .unwrap(); assert_eq!(x, SetterOptOut { @@ -114,15 +116,20 @@ fn setter_opt_in() { #[test] #[cfg(feature = "struct_default")] fn setter_skipped_with_struct_default() { - let x = SetterOptInStructDefaultBuilder::default().build().unwrap(); + let x = SetterOptInStructDefaultBuilder::default() + .build() + .unwrap(); assert_eq!(x, SetterOptInStructDefault::default()); } #[test] fn setter_skipped_with_field_default() { - let x = SetterOptInFieldDefaultBuilder::default().build().expect("All fields were defaulted"); - assert_eq!(x, SetterOptInFieldDefault { - setter_skipped_with_field_default: new_notdefaultable(), - setter_present_by_default: Default::default(), - }); -} \ No newline at end of file + let x = SetterOptInFieldDefaultBuilder::default() + .build() + .expect("All fields were defaulted"); + assert_eq!(x, + SetterOptInFieldDefault { + setter_skipped_with_field_default: new_notdefaultable(), + setter_present_by_default: Default::default(), + }); +} diff --git a/derive_builder/tests/try_setter.rs b/derive_builder/tests/try_setter.rs index 109ea332..f74fd366 100644 --- a/derive_builder/tests/try_setter.rs +++ b/derive_builder/tests/try_setter.rs @@ -37,7 +37,7 @@ struct Lorem { #[derive(Debug, PartialEq, Builder)] #[builder(try_setter, setter(into, prefix = "set"))] struct Ipsum { - pub source: MyAddr + pub source: MyAddr, } fn exact_helper() -> Result { @@ -68,7 +68,8 @@ fn infallible_set() { fn fallible_set() { let mut builder = LoremBuilder::default(); let try_result = builder.try_source("1.2.3.4"); - let built = try_result.expect("Passed well-formed address") + let built = try_result + .expect("Passed well-formed address") .dest(IpAddr::from_str("0.0.0.0").unwrap()) .build() .unwrap(); diff --git a/derive_builder_core/src/bindings.rs b/derive_builder_core/src/bindings.rs index 6edb3fa1..78527dc1 100644 --- a/derive_builder_core/src/bindings.rs +++ b/derive_builder_core/src/bindings.rs @@ -4,149 +4,122 @@ use RawTokens; #[derive(Debug, Clone, Copy, Default)] pub struct Bindings { /// Whether the generated code should comply with `#![no_std]`. - pub no_std: bool + pub no_std: bool, } impl Bindings { /// String type. pub fn string_ty(&self) -> RawTokens<&'static str> { RawTokens(if self.no_std { - ":: collections :: string :: String" - } else { - ":: std :: string :: String" - }) + ":: collections :: string :: String" + } else { + ":: std :: string :: String" + }) } /// Result type. pub fn result_ty(&self) -> RawTokens<&'static str> { RawTokens(if self.no_std { - ":: core :: result :: Result" - } else { - ":: std :: result :: Result" - }) + ":: core :: result :: Result" + } else { + ":: std :: result :: Result" + }) } /// Option type. pub fn option_ty(&self) -> RawTokens<&'static str> { RawTokens(if self.no_std { - ":: core :: option :: Option" - } else { - ":: std :: option :: Option" - }) + ":: core :: option :: Option" + } else { + ":: std :: option :: Option" + }) } /// PhantomData type. pub fn phantom_data_ty(&self) -> RawTokens<&'static str> { RawTokens(if self.no_std { - ":: core :: marker :: PhantomData" - } else { - ":: std :: marker :: PhantomData" - }) + ":: core :: marker :: PhantomData" + } else { + ":: std :: marker :: PhantomData" + }) } /// Default trait. pub fn default_trait(&self) -> RawTokens<&'static str> { RawTokens(if self.no_std { - ":: core :: default :: Default" - } else { - ":: std :: default :: Default" - }) + ":: core :: default :: Default" + } else { + ":: std :: default :: Default" + }) } /// Clone trait. pub fn clone_trait(&self) -> RawTokens<&'static str> { RawTokens(if self.no_std { - ":: core :: clone :: Clone" - } else { - ":: std :: clone :: Clone" - }) + ":: core :: clone :: Clone" + } else { + ":: std :: clone :: Clone" + }) } /// Into trait. pub fn into_trait(&self) -> RawTokens<&'static str> { RawTokens(if self.no_std { - ":: core :: convert :: Into" - } else { - ":: std :: convert :: Into" - }) + ":: core :: convert :: Into" + } else { + ":: std :: convert :: Into" + }) } - + /// TryInto trait. pub fn try_into_trait(&self) -> RawTokens<&'static str> { RawTokens(if self.no_std { - ":: core :: convert :: TryInto" - } else { - ":: std :: convert :: TryInto" - }) + ":: core :: convert :: TryInto" + } else { + ":: std :: convert :: TryInto" + }) } } #[test] fn std() { - let b = Bindings { - no_std: false - }; + let b = Bindings { no_std: false }; - assert_eq!(b.string_ty().to_tokens(), quote!( - ::std::string::String - )); + assert_eq!(b.string_ty().to_tokens(), quote!(::std::string::String)); - assert_eq!(b.result_ty().to_tokens(), quote!( - ::std::result::Result - )); + assert_eq!(b.result_ty().to_tokens(), quote!(::std::result::Result)); - assert_eq!(b.option_ty().to_tokens(), quote!( - ::std::option::Option - )); + assert_eq!(b.option_ty().to_tokens(), quote!(::std::option::Option)); - assert_eq!(b.phantom_data_ty().to_tokens(), quote!( - ::std::marker::PhantomData - )); + assert_eq!(b.phantom_data_ty().to_tokens(), + quote!(::std::marker::PhantomData)); - assert_eq!(b.default_trait().to_tokens(), quote!( - ::std::default::Default - )); + assert_eq!(b.default_trait().to_tokens(), + quote!(::std::default::Default)); - assert_eq!(b.clone_trait().to_tokens(), quote!( - ::std::clone::Clone - )); + assert_eq!(b.clone_trait().to_tokens(), quote!(::std::clone::Clone)); - assert_eq!(b.into_trait().to_tokens(), quote!( - ::std::convert::Into - )); + assert_eq!(b.into_trait().to_tokens(), quote!(::std::convert::Into)); } #[test] fn no_std() { - let b = Bindings { - no_std: true - }; + let b = Bindings { no_std: true }; - assert_eq!(b.string_ty().to_tokens(), quote!( - ::collections::string::String - )); + assert_eq!(b.string_ty().to_tokens(), + quote!(::collections::string::String)); - assert_eq!(b.result_ty().to_tokens(), quote!( - ::core::result::Result - )); + assert_eq!(b.result_ty().to_tokens(), quote!(::core::result::Result)); - assert_eq!(b.option_ty().to_tokens(), quote!( - ::core::option::Option - )); + assert_eq!(b.option_ty().to_tokens(), quote!(::core::option::Option)); - assert_eq!(b.phantom_data_ty().to_tokens(), quote!( - ::core::marker::PhantomData - )); + assert_eq!(b.phantom_data_ty().to_tokens(), + quote!(::core::marker::PhantomData)); - assert_eq!(b.default_trait().to_tokens(), quote!( - ::core::default::Default - )); + assert_eq!(b.default_trait().to_tokens(), + quote!(::core::default::Default)); - assert_eq!(b.clone_trait().to_tokens(), quote!( - ::core::clone::Clone - )); + assert_eq!(b.clone_trait().to_tokens(), quote!(::core::clone::Clone)); - assert_eq!(b.into_trait().to_tokens(), quote!( - ::core::convert::Into - )); + assert_eq!(b.into_trait().to_tokens(), quote!(::core::convert::Into)); } diff --git a/derive_builder_core/src/build_method.rs b/derive_builder_core/src/build_method.rs index 28ade31f..9059b5eb 100644 --- a/derive_builder_core/src/build_method.rs +++ b/derive_builder_core/src/build_method.rs @@ -76,9 +76,9 @@ impl<'a> ToTokens for BuildMethod<'a> { let default_struct = self.default_struct .as_ref() .map(|default_expr| { - let ident = syn::Ident::new(DEFAULT_STRUCT_NAME); - quote!(let #ident: #target_ty = #default_expr;) - }); + let ident = syn::Ident::new(DEFAULT_STRUCT_NAME); + quote!(let #ident: #target_ty = #default_expr;) + }); let result = self.bindings.result_ty(); let string = self.bindings.string_ty(); diff --git a/derive_builder_core/src/builder.rs b/derive_builder_core/src/builder.rs index 8cae3e46..265cf21c 100644 --- a/derive_builder_core/src/builder.rs +++ b/derive_builder_core/src/builder.rs @@ -74,7 +74,10 @@ impl<'a> ToTokens for Builder<'a> { let builder_doc_comment = &self.doc_comment; let deprecation_notes = &self.deprecation_notes.as_item(); - debug!("ty_generics={:?}, where_clause={:?}, impl_generics={:?}", ty_generics, where_clause, impl_generics); + debug!("ty_generics={:?}, where_clause={:?}, impl_generics={:?}", + ty_generics, + where_clause, + impl_generics); tokens.append(quote!( #[derive(Default, Clone)] diff --git a/derive_builder_core/src/initializer.rs b/derive_builder_core/src/initializer.rs index 7b7570e7..886d6ad3 100644 --- a/derive_builder_core/src/initializer.rs +++ b/derive_builder_core/src/initializer.rs @@ -82,10 +82,12 @@ impl<'a> Initializer<'a> { match self.builder_pattern { BuilderPattern::Owned => MatchSome::Move, BuilderPattern::Mutable | - BuilderPattern::Immutable => if self.bindings.no_std { - MatchSome::CloneNoStd - } else { - MatchSome::Clone + BuilderPattern::Immutable => { + if self.bindings.no_std { + MatchSome::CloneNoStd + } else { + MatchSome::Clone + } }, } } @@ -94,17 +96,20 @@ impl<'a> Initializer<'a> { fn match_none(&'a self) -> MatchNone<'a> { match self.default_value { Some(ref expr) => MatchNone::DefaultTo(expr), - None => if self.use_default_struct { - MatchNone::UseDefaultStructField(self.field_ident) - } else if self.bindings.no_std { - MatchNone::ReturnErrorNoStd(format!("`{}` must be initialized", self.field_ident)) - } else { - MatchNone::ReturnError(format!("`{}` must be initialized", self.field_ident)) + None => { + if self.use_default_struct { + MatchNone::UseDefaultStructField(self.field_ident) + } else if self.bindings.no_std { + MatchNone::ReturnErrorNoStd(format!("`{}` must be initialized", + self.field_ident)) + } else { + MatchNone::ReturnError(format!("`{}` must be initialized", self.field_ident)) + } } } } - fn default(&'a self) -> Tokens { + fn default(&'a self) -> Tokens { match self.default_value { Some(ref expr) => quote!(#expr), None if self.use_default_struct => { diff --git a/derive_builder_core/src/setter.rs b/derive_builder_core/src/setter.rs index 0875f34b..ac75516c 100644 --- a/derive_builder_core/src/setter.rs +++ b/derive_builder_core/src/setter.rs @@ -119,13 +119,13 @@ impl<'a> ToTokens for Setter<'a> { new.#field_ident = #option::Some(#into_value); new })); - + if self.try_setter { let try_into = self.bindings.try_into_trait(); let try_ty_params = quote!(>); let try_ident = syn::Ident::new(format!("try_{}", ident)); let result = self.bindings.result_ty(); - + tokens.append(quote!( #(#attrs)* #vis fn #try_ident #try_ty_params (#self_param, value: VALUE) @@ -266,9 +266,9 @@ mod tests { new.foo = ::std::option::Option::Some(value.into()); new } - + #[some_attr] - pub fn try_foo>(&mut self, value: VALUE) + pub fn try_foo>(&mut self, value: VALUE) -> ::std::result::Result<&mut Self, VALUE::Error> { let converted : Foo = value.try_into()?; let mut new = self; @@ -315,21 +315,21 @@ mod tests { assert_eq!(quote!(#setter), quote!()); } - + #[test] fn try_setter() { let mut setter: Setter = default_setter!(); setter.pattern = BuilderPattern::Mutable; setter.try_setter = true; - + assert_eq!(quote!(#setter), quote!( pub fn foo(&mut self, value: Foo) -> &mut Self { let mut new = self; new.foo = ::std::option::Option::Some(value); new } - - pub fn try_foo>(&mut self, value: VALUE) + + pub fn try_foo>(&mut self, value: VALUE) -> ::std::result::Result<&mut Self, VALUE::Error> { let converted : Foo = value.try_into()?; let mut new = self; diff --git a/derive_builder_core/src/tokens.rs b/derive_builder_core/src/tokens.rs index 2fa4dcc6..ebab1a8c 100644 --- a/derive_builder_core/src/tokens.rs +++ b/derive_builder_core/src/tokens.rs @@ -1,6 +1,6 @@ use quote::{Tokens, ToTokens}; -/// RawTokens can be directly appended to a `quote::Tokens` instance without any parsing. +/// `RawTokens` can be directly appended to a `quote::Tokens` instance without any parsing. #[derive(PartialEq, Debug)] pub struct RawTokens>(pub T); From 14f4bba5ee868359f76e44bb059fe3ea9ef07a9e Mon Sep 17 00:00:00 2001 From: Colin Kiegel Date: Sat, 15 Apr 2017 22:02:49 +0200 Subject: [PATCH 07/11] cleanup --- derive_builder/src/lib.rs | 8 +++++--- derive_builder/tests/setter_visibility.rs | 4 ++-- derive_builder_core/src/deprecation_notes.rs | 4 ++-- derive_builder_core/src/doc_comment.rs | 2 +- derive_builder_core/src/setter.rs | 2 +- 5 files changed, 11 insertions(+), 9 deletions(-) diff --git a/derive_builder/src/lib.rs b/derive_builder/src/lib.rs index 970c6805..83ba3d0b 100644 --- a/derive_builder/src/lib.rs +++ b/derive_builder/src/lib.rs @@ -412,16 +412,18 @@ //! //! - Tuple structs and unit structs are not supported as they have no field //! names. +//! - Generic structs need the boundary `where T: std::clone::Clone` if +//! used in combination with the immutable/mutable pattern //! - Generic setters introduce a type parameter `VALUE: Into<_>`. Therefore you can't use //! `VALUE` as a type parameter on a generic struct in combination with generic setters. +//! - The `try_setter` attribute and `owned` builder pattern are not compatible in practice; +//! an error during building will consume the builder, making it impossible to continue +//! construction. //! - When re-exporting the underlying struct under a different name, the //! auto-generated documentation will not match. //! - If derive_builder depends on your crate, and vice versa, then a cyclic //! dependency would occur. To break it you could try to depend on the //! [`derive_builder_core`] crate instead. -//! - The `try_setter` attribute and `owned` builder pattern are not compatible in practice; -//! an error during building will consume the builder, making it impossible to continue -//! construction. //! //! ## Debugging Info //! diff --git a/derive_builder/tests/setter_visibility.rs b/derive_builder/tests/setter_visibility.rs index 92ff2cad..3b93b592 100644 --- a/derive_builder/tests/setter_visibility.rs +++ b/derive_builder/tests/setter_visibility.rs @@ -56,7 +56,7 @@ fn public_setters_override_foreign_module() { .build() .unwrap(); - assert_eq!(x.public, String::from("Hello")); + assert_eq!(x.public, "Hello".to_string()); } #[test] @@ -67,7 +67,7 @@ fn public_setters_foreign_module() { .build() .unwrap(); - assert_eq!(y.public, String::from("Hello")); + assert_eq!(y.public, "Hello".to_string()); } // compile-test should fail with "error: method `ipsum` is private" diff --git a/derive_builder_core/src/deprecation_notes.rs b/derive_builder_core/src/deprecation_notes.rs index bd18ef5f..c9689003 100644 --- a/derive_builder_core/src/deprecation_notes.rs +++ b/derive_builder_core/src/deprecation_notes.rs @@ -17,7 +17,7 @@ use syn; /// # use derive_builder_core::DeprecationNotes; /// # fn main() { /// # let mut note = DeprecationNotes::default(); -/// # note.push(String::from("Some Warning")); +/// # note.push("Some Warning".to_string()); /// # assert_eq!(quote!(#note), quote!( /// { /// #[deprecated(note="Some Warning")] @@ -93,7 +93,7 @@ impl<'a> ToTokens for DeprecationNotesAsItem<'a> { #[test] fn deprecation_note() { let mut note = DeprecationNotes::default(); - note.push(String::from("Some Warning")); + note.push("Some Warning".to_string()); assert_eq!(quote!(#note), quote!( { #[deprecated(note="Some Warning")] diff --git a/derive_builder_core/src/doc_comment.rs b/derive_builder_core/src/doc_comment.rs index 6dc68a76..aebd0837 100644 --- a/derive_builder_core/src/doc_comment.rs +++ b/derive_builder_core/src/doc_comment.rs @@ -13,7 +13,7 @@ use syn; /// # extern crate derive_builder_core; /// # use derive_builder_core::doc_comment_from; /// # fn main() { -/// # let doc_comment = doc_comment_from(String::from("foo")); +/// # let doc_comment = doc_comment_from("foo".to_string()); /// # /// # assert_eq!(quote!(#doc_comment), quote!( /// #[doc = r##"foo"##] diff --git a/derive_builder_core/src/setter.rs b/derive_builder_core/src/setter.rs index ac75516c..81cdaaf4 100644 --- a/derive_builder_core/src/setter.rs +++ b/derive_builder_core/src/setter.rs @@ -250,7 +250,7 @@ mod tests { let attrs = vec![syn::parse_outer_attr("#[some_attr]").unwrap()]; let mut deprecated = DeprecationNotes::default(); - deprecated.push(String::from("Some example.")); + deprecated.push("Some example.".to_string()); let mut setter = default_setter!(); setter.attrs = attrs.as_slice(); From e48b24d040450a5253c3350f50bef2f07d0d2e3d Mon Sep 17 00:00:00 2001 From: Colin Kiegel Date: Sat, 15 Apr 2017 22:16:42 +0200 Subject: [PATCH 08/11] clippy [ci skip] --- derive_builder_core/src/deprecation_notes.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/derive_builder_core/src/deprecation_notes.rs b/derive_builder_core/src/deprecation_notes.rs index c9689003..a30ce056 100644 --- a/derive_builder_core/src/deprecation_notes.rs +++ b/derive_builder_core/src/deprecation_notes.rs @@ -64,8 +64,8 @@ impl DeprecationNotes { } /// Create a view of these deprecation notes that can annotate a struct. - pub fn as_item<'a>(&'a self) -> DeprecationNotesAsItem<'a> { - DeprecationNotesAsItem(&self) + pub fn as_item(&self) -> DeprecationNotesAsItem { + DeprecationNotesAsItem(self) } } From 27475d3037a12cde37423f582fe131794425efb5 Mon Sep 17 00:00:00 2001 From: Ted Driggs Date: Fri, 14 Apr 2017 10:27:23 -0700 Subject: [PATCH 09/11] Vision proposal --- vision_doc.md | 279 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 279 insertions(+) create mode 100644 vision_doc.md diff --git a/vision_doc.md b/vision_doc.md new file mode 100644 index 00000000..4bd36b4a --- /dev/null +++ b/vision_doc.md @@ -0,0 +1,279 @@ +# Vision +`derive_builder` provides an ergonomic way to deal with construction of guaranteed-complete objects from a range of inputs: + +* Statically-known values +* 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. + +## 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. + +## 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). + +## 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. + +In this proposal, an infallible builder is one which declares `#[builder(default)]` at the struct level and does not skip the automatic build function, which requires that all fields be optional and any well-formed value be acceptable. When a builder meets these criteria, a `From<&mut TargetBuilder> for Target` impl would be automatically emitted (exact implementations emitted would depend on builder pattern). **Open question: does this change the signature of the emitted `build` method, add a new inherent method, or just add the `Into` implementation?** + +To handle the mix-required-and-optional case, exporting crate would refactor to have a `Target` struct and a `TargetOptions` struct, with a builder derived on one or both of those. The `TargetOptions` struct would have only optional fields, thus enabling an infallible build of it. + +# Use Case +Consider a `ConnectionPool` object. It has: + +* A set of required fields, such as `host`, `api_key`, etc. +* A set of optional/defaulted fields: `timeout`, `retry_policy`, etc. +* A private TCP connection that is opened in `ConnectionPool::new` and closed during drop + +This connection pool object's data is pulled from multiple sources, in decreasing priority: + +1. Command-line args +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. + +Under this vision proposal, `ConnectionPool` would be declared as follows: + +```rust + +#[derive(Debug, Builder, Serialize)] +#[builder(name = "ConnectionInfo", + setter(into), + try_setter, + build_fn(skip), + derive(Deserialize), + preserve_attrs(serde))] +pub struct ConnectionPool { + host: String, + + api_key: ApiKey, + + #[builder(default="ConnectionPool::default_timeout()")] + timeout: Duration, + + #[builder(setter(skip))] + #[serde(skip_serializing)] + socket: Socket +} + +impl ConnectionPool { + /// Could be private if the builder was meant to be only way to create instance. + pub fn new(host: String, api_key: ApiKey, timeout: Duration) -> Result { + // @FAERN; validation would occur here + + let socket = unimplemented!(); // try to open the socket... + ConnectionPool { + host: host, + api_key: api_key, + timeout: timeout + } + } + + fn default_timeout() -> Duration { + Duration::from_millis(5000) + } +} + +impl ConnectionInfo { + /// Establish a connection using the given information. + pub fn connect(&self) -> Result { + ConnectionPool::new( + self.get_host()?, + self.get_api_key()?, + self.get_timeout()?) + ) + } +} +``` + +# Example +```rust +mod exporting_crate { + use std::net::IpAddr; + use serde::{Serialize, Deserialize}; + + #[derive(Debug, Clone, Copy, Serialize, Deserialize)] + #[serde(rename_all = "snake_case")] + pub enum Cycle { + Auto, + Slow, + Medium, + Fast, + } + + impl FromStr for Cycle { + type Err = String; + // ... impl elided + } + + #[derive(Debug, Clone, Serialize, Deserialize)] + #[serde(tag = "type", rename_all = "snake_case")] + pub enum Key { + Str(String), + IpAddr(IpAddr), + } + + #[derive(Debug, Clone)] + pub enum Filter { + Approx(String), + Exact(Key), + } + + impl Serialize for Filter { + // ... impl elided + } + + impl Deserialize for Filter { + // ... impl elided + } + + /// A data request for the Northwind metrics API. + /// This type is focused on modeling a well-formed stat + /// request, it doesn't allow for partially-built states. + /// + /// This invents a few new properties for the builder macro: + /// + /// * `build_fn(skip)`: This tells the builder not to auto-generate the + /// build method. + /// * `preserve_attributes`: This tells the builder to include all #[serde] + /// attributes on the `StatRequestBuilder` fields. + /// * `target_factory`: Name needs work, but this tells the macro to add an + /// 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 { + /// This particular field should be named `stat` on the inbound request; + /// that requires a field-level annotation. + #[serde(rename(deserialize = "stat"))] + stat_name: String, + + cycle: Cycle, + + #[serde(default, rename = "filter", skip_serializing_if="Option::is_none")] + #[builder(default)] + key_filter: Option, + } + + impl StatRequest { + pub fn new(stat_name: String, cycle: Cycle) -> Result { + StatRequest::with_filter(stat_name, cycle, None) + } + + /// Creates a new `StatRequest` with the specified filter (`None` is allowed). + /// + /// A filter is required for detail stats for perf reasons; this method will + /// return an error if one is needed and `None` is provided, or if one is provided + /// and the specified stat does not support filtering. + pub fn with_filter>>(stat_name: String, + cycle: Cycle, + filter: F) + -> Result { + let requires_filter = stat_name.ends_with("_detail"); + let filter_opt = filter.into(); + match (requires_filter, filter_opt.is_some()) { + (true, false) => Err(format!("Stat `{}` requires a filter", stat_name)), + (false, true) => Err(format!("Stat `{}` is not a detail metric")), + _ => { + Ok(StatRequest { + stat_name: stat_name, + key_filter: filter_opt, + cycle: cycle, + }) + } + } + } + } + + impl StatRequestBuilder { + /// Create a stat request by calling `StatRequest::with_filter`. This method + /// will return an error if a required field is not initialized, or if the + /// values fail validation. + pub fn build(&self) -> Result { + // These getters are private methods which return Result; + // the error arises if the field is uninitialized and has no default. + StatRequest::with_filter( + self.get_stat_name()?, + self.get_cycle()?, + self.get_filter()?) + } + } + + impl TryFrom for StatRequest { + type Error = String; + + fn try_from(v: StatRequestBuilder) -> Result { + v.build() + } + } +} + +mod consuming_crate { + mod errors { + error_chain! { + // ... errors elided + } + } + + use errors::{Error, ChainedError, Result, ResultExt}; + use exporting_crate::{Cycle, Key, Filter, StatRequest}; + + fn merge_args(inbound_req: &str, cycle_lock: Option) -> Result { + // deserialize from the inbound request into a builder; this enables further + // validation and can be used to generate nicer failures if required fields are + // missing. This is using error_chain and the carrier operator's conversion + // feature to be very brief while still returning a strongly-typed error specific + // to the consuming crate. + let mut builder = serde_json::from_str(req_body)?; + + // apply the cycle override, if one was provided. + if let Some(cl) = cycle_lock { + builder.cycle(cl); + } + + builder.build() + } + + fn request(query: &StatRequest) -> Result<()> { + let rsp = reqwest::Client::new()? + .post("https://northwind.com/api/v1/metrics") + .json(&query) + .send()?; + + rsp.validate_status() + .map(|| ()) + .chain_err(|| "The server returned an error") + } + + /// Run an arbitrary stat request. + pub fn run(req: &str) -> Result<()> { + // Look for an environment-defined cycle lock and parse it if found. + let cycle_lock = dotenv::var("CYCLE_LOCK").map(Cycle::from_str)?; + + // create the StatRequest; still relying on the error_chain + carrier pair + // seen above. Now, `def` is a `StatRequest` which is ready to use. + let query = merge_args(req, cycle_lock)?; + + request(query) + } + + /// Test if the service is up or down. + pub fn health_check(cycle: Cycle) -> Result<()> { + // this gets a fresh StatRequestBuilder, despite not having + // imported the + let query = StatRequest::builder() + .stat_name("system.is_up") + .cycle(cycle) + .build() + .chain_err(|| "Health check is malformed")?; + + request(query); + } +} +``` \ No newline at end of file From 6912f3bcc5dc9c4d4edb9e5d744e0289a1c5cb65 Mon Sep 17 00:00:00 2001 From: Ted Driggs Date: Mon, 17 Apr 2017 12:17:13 -0700 Subject: [PATCH 10/11] Updated to address feedback --- vision_doc.md | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/vision_doc.md b/vision_doc.md index 4bd36b4a..ef39ab1e 100644 --- a/vision_doc.md +++ b/vision_doc.md @@ -1,11 +1,11 @@ # Vision -`derive_builder` provides an ergonomic way to deal with construction of guaranteed-complete objects from a range of inputs: +`derive_builder` provides an ergonomic and [idiomatic] way to deal with construction of guaranteed-complete objects from a range of inputs: * Statically-known values * 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. +A builder should produce only objects that uphold the invariants of the target type. This includes, but is not limited to, checks upheld by the type system. ## Documentation Authors should focus documentation efforts on the target type. @@ -15,14 +15,10 @@ Authors should focus documentation efforts on the target type. ## 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). +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 observed by the language server, 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. - -In this proposal, an infallible builder is one which declares `#[builder(default)]` at the struct level and does not skip the automatic build function, which requires that all fields be optional and any well-formed value be acceptable. When a builder meets these criteria, a `From<&mut TargetBuilder> for Target` impl would be automatically emitted (exact implementations emitted would depend on builder pattern). **Open question: does this change the signature of the emitted `build` method, add a new inherent method, or just add the `Into` implementation?** - -To handle the mix-required-and-optional case, exporting crate would refactor to have a `Target` struct and a `TargetOptions` struct, with a builder derived on one or both of those. The `TargetOptions` struct would have only optional fields, thus enabling an infallible build of it. +The typesafe builder case (which is capable of statically validating that all required fields are present) can be achieved using a generic which marks when all required fields are present. A constructor on the builder can take non-optional values for all the required fields, and return a builder with the correct session type. By exposing inherent and trait `impl` blocks only on the generic instantiation with the correct state as its type parameter, the same implementation can serve both purposes. # Use Case Consider a `ConnectionPool` object. It has: @@ -66,13 +62,14 @@ pub struct ConnectionPool { impl ConnectionPool { /// Could be private if the builder was meant to be only way to create instance. pub fn new(host: String, api_key: ApiKey, timeout: Duration) -> Result { - // @FAERN; validation would occur here + // validation would occur here let socket = unimplemented!(); // try to open the socket... ConnectionPool { host: host, api_key: api_key, - timeout: timeout + timeout: timeout, + socket: socket, } } @@ -147,7 +144,7 @@ mod exporting_crate { /// * `target_factory`: Name needs work, but this tells the macro to add an /// 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)] + #[builder(setter(into), try_setter, preserve_attrs(serde), derive(Deserialize), build_fn(skip), target_factory)] pub struct StatRequest { /// This particular field should be named `stat` on the inbound request; /// that requires a field-level annotation. @@ -276,4 +273,8 @@ mod consuming_crate { request(query); } } -``` \ No newline at end of file +``` + +[idiomatic]: https://aturon.github.io/ownership/builders.html +["find all references"]: https://github.com/rust-lang-nursery/rls/blob/master/src/server.rs#L162 +[show docs for the target type on type hover]: https://github.com/rust-lang-nursery/rls/blob/master/src/server.rs#L160 \ No newline at end of file From e515cd4fecefe004de286db919619d659a116128 Mon Sep 17 00:00:00 2001 From: Ted Driggs Date: Mon, 17 Apr 2017 12:21:19 -0700 Subject: [PATCH 11/11] Added language server protocol link --- vision_doc.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/vision_doc.md b/vision_doc.md index ef39ab1e..060ec89d 100644 --- a/vision_doc.md +++ b/vision_doc.md @@ -10,16 +10,19 @@ A builder should produce only objects that uphold the invariants of the target t ## 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. +* The builder can - in most cases - 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. -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 observed by the language server, 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). +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 observed by the language server, 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). For more information on this, see the [language server protocol] that RLS is implementing. ## Fallible vs. Infallible Builders The typesafe builder case (which is capable of statically validating that all required fields are present) can be achieved using a generic which marks when all required fields are present. A constructor on the builder can take non-optional values for all the required fields, and return a builder with the correct session type. By exposing inherent and trait `impl` blocks only on the generic instantiation with the correct state as its type parameter, the same implementation can serve both purposes. +## Validation +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. + # Use Case Consider a `ConnectionPool` object. It has: @@ -277,4 +280,5 @@ mod consuming_crate { [idiomatic]: https://aturon.github.io/ownership/builders.html ["find all references"]: https://github.com/rust-lang-nursery/rls/blob/master/src/server.rs#L162 -[show docs for the target type on type hover]: https://github.com/rust-lang-nursery/rls/blob/master/src/server.rs#L160 \ No newline at end of file +[show docs for the target type on type hover]: https://github.com/rust-lang-nursery/rls/blob/master/src/server.rs#L160 +[language server protocol]: https://github.com/Microsoft/language-server-protocol \ No newline at end of file