Miscellaneous fairly extensive improvements and changes #16
Conversation
For required fields, you could already access the value as `self.foo.0`,
and for other fields by abusing implementation details, but that was all
a really nasty way of doing it. This is much nicer.
As noted in the changelog, this is a breaking change, but extremely
unlikely to actually break any code, in part due to case conventions.
Here’s a simple demonstration of the sort of code that it would break
(in this case silently, but compilation failure is also possible if the
shadowed value is of a different type):
pub const x: i32 = 1;
struct Foo {
x: i32,
#[builder(default_code = "x")]
y: i32,
}
Foo::builder().x(2).build().y
Before this commit, y will be 1, from the x const.
After this commit, y will be 2, from the x field.
One mildly discomfiting aspect of this feature is that it allows the
order of field declaration to be significant. As noted in the code, we
can fix this later with a field dependency DAG guiding reordering. That
would also be a technically breaking change for the same reasons as this
commit is, but given this feature you’d have to be somewhat insane to
write code that would be broken by it!
A reasonable extension of this work is allowing the user to add prelude
statements to the build() method, so values to be used by the default
calculations of multiple methods can be defined.
An alternative I considered to avoid this being a breaking change (if we
consider current use of `self` to be not public API, and thus fair game
to break) is giving the locals namespaced names so that `foo` would be
`TypeBuilder_field_foo`, transforming `default_code` so that `self.foo`
would become `TypedBuilder_field_foo`. But that’s harder, and an abuse
of `self` anyway. Better, I think, to do it this way.
I think I’ve added enough useful functionality to warrant my presence.
This doesn’t actually solve idanarye#11 (there are still substantial changes that should be made to the default docs for the `builder()` method), but it lets you customise how it will appear, and unhide the builder type, which are both a Good Thing™. I have written no real tests for this because the system’s not well-placed for doing that; I’ve just written tests to ensure that it at least compiles. Yet not testing the documentation generated is not good.
I expect this to cause no trouble at all, and it’s a much nicer default. I see no reason for “TypedBuilder” to appear anywhere in the generated code or docs. `#[builder(name = TypedBuilder_BuilderFor_Foo)]` will restore the old behaviour if anyone really needs or wants it for some bizarre reason. I tossed up between `#[builder(name = "StringLiteral")]` and `#[builder(name = Identifier)]` forms; historically, attributes with `=` in Rust were all `key = "value"`, and identifier values were only used in the `function(value)` style (e.g. with `derive(…)` and `cfg(…)`). (This was actually a syntactic rule; the meta matcher used to be much tighter, but was relaxed a lot somewhere along the way, not long before Rust 1.0 if I recall correctly.) Although this is still the general convention, it’s weaker than it used to be, and this is a case where I think breaking the convention is probably warranted, as the value will be used as an identifier in the generated code. In the end, Identifier was what I implemented, completely by accident (I confess I thought I’d implemented the other), so I stuck with it.
CHANGELOG.md has the explanation.
This change is equivalent to replacing `Option::unwrap_or` with `Option::unwrap_or_else`.
Only one more mention of “TypedBuilder” is left in the generated code, `_TypedBuilder__phantomGenerics_`.
I guess it used to be called that.
I see no reason for the restriction, and it’s not idiomatic.
When there was just BuilderAttr, it being separate made some sense. Now there is TypeBuilderAttr to join FieldBuilderAttr, keeping them together makes little sense, so it’s a question of splitting them into their own files, or keeping them together with their Info struct. I think keeping them together makes sense.
There are places where it makes it nicer, and places where it makes it worse, but a consistent style is still generally worthwhile. I applied one manual modification to what rustfmt did: to neaten the string line breaks and indentation in the `#[builder(doc = "…")]` example in tests/tests.rs.
The difference is negligible, but I like the generated code to be neat, completely human-readable, and directly maintainable where possible.
`_TypedBuilder__phantomGenerics_` → `__typed_builder_phantom_generics`. Snake case instead of a mix of PascalCase and camelCase (the latter of which especially is entirely unprecedented in Rust). There’s much precedent in the Rust ecosystem for a leading double underscore for things like this. (Frankly I’d call even the “typed_builder” part unnecessary, but it’s fairly harmless.)
|
Haven't had the chance to go through all these commits (I only skimmed through the titles) but it looks like many of them are about make the builder types more ergonomic? The builder types are non-ergonomic as a sort of poor man's macro hygiene. I don't want people to think they can use them, because they need weird complex generic arguments that change every time you set a field. Unlike derive_builder's builder, it is not a type you can easily pass around and play with. I also gave them weird names (based on Python's private mangling scheme - though I didn't put the leading |
| }; | ||
| Ok(data) | ||
| } | ||
|
|
||
| // It’d be nice for the compilation tests to live in tests/ with the rest, but short of pulling in | ||
| // some other test runner for that purpose (e.g. compiletest_rs), rustdoc compile_fail in this |
There was a problem hiding this comment.
I actually wouldn't mind adding compiletest_rs. I was unaware of its existence (ashamed to say I didn't bother to search for something like this), but for this kind of crate it can be very useful, and allow writing tests for many currently manually-tested (==untested) scenarios.
There was a problem hiding this comment.
I agree, adding compiletest_rs seems like a good idea. I don’t plan to block this on it, though.
| - `#[builder(exclude)]` on fields, to not provide a method to set that field. | ||
| - Control of documentation: | ||
| - `#[builder(doc = "…")]` on fields, to document the field’s method on the builder. Unlike `#[doc]`, you can currently only have one value rather than one attribute per line; but that’s not a big deal since you don’t get to use the `///` sugar anyway. Just use a multiline string. | ||
| - `#[builder(doc, builder_method_doc = "…", builder_type_doc = "…", build_method_doc = "…")]` on structs: |
There was a problem hiding this comment.
This style seems a bit cumbersome... wouldn't it be better to use nested attributes?
#[builder(builder_method_doc = "...", builder_type_doc(type_doc = "...", build_method_doc = "..."))]And when you want to automatically create the docs for the builder type, just use:
#[builder(builder_method_doc = "...", builder_type_doc())]or even
#[builder(builder_method_doc = "...", builder_type_doc)]There was a problem hiding this comment.
Well, I had doc as implied by either of the other two, so it would normally either be #[builder(doc)] or the likes of #[builder(builder_type_doc = "…")].
So many things starting with the letters “build” is bad for health!
We could do it with nested attributes, but I don’t think it’s particularly better or worse. Flat can be easier to read and remember.
|
|
||
| ### Changed | ||
| - [**BREAKING**] Renamed the generated builder type from `TypedBuilder_BuilderFor_Foo` to `FooBuilder`, for improved ergonomics, especially when you enable documentation of the builder type. You can also now change it to something else with `#[builder(name = SomethingElse)]` on the type you are deriving TypedBuilder on. | ||
| - Generic identifiers were also changed, from `TypedBuilder_genericType_x` to `__x`. This is still expected to avoid all name collisions, but is easier to read in the builder type docs if you enable them. |
There was a problem hiding this comment.
How is __x avoiding all name collisions?
There was a problem hiding this comment.
Prefixing a generic with double underscore is an existing convention for avoiding collisions with other generics. For example, Serde Serialize and Deserialize deriving use it (__S, __D, used on some other values that I can’t recall off the top of my head too). User code should not be using double underscore prefixes on anything ever, so collisions are expected to be avoided.
There was a problem hiding this comment.
And collisions with identifiers created by other proc-macros?
There was a problem hiding this comment.
The generics are only on the FooBuilder type, not the Foo type. You can’t #[derive] things or put custom attributes on the builder, so I’m content to say the practical scope for collision is nil. (Hmm, I wonder whether #[builder_attr(derive(Clone))] putting #[derive(Clone)] on the builder type would allow it to work. Should experiment with that before committing to this.)
| args.push(syn::GenericArgument::Type(empty_type())); | ||
| } | ||
| }); | ||
| let skip_phantom_generics = self.generics.params.is_empty(); |
There was a problem hiding this comment.
Not sure if I agree with your reasoning here. This is a generated field that we won't have to touch and the users won't have to see, plus it's private so it won't appear in the docs. I think uniformity is more important than neatness in these cases.
There was a problem hiding this comment.
This is also the change among them that I’m most dubious about. As it stands, it doesn’t make a great deal of sense, but with what I have in mind in the way of considering the builder a more public thing and worthy of extension, maybe it makes sense. Then again, I suppose such extensions can use the existing methods too, so it’s probably not worthwhile after all. Yeah, in the end I’m inclined to agree with you, the increase in code complexity is not warranted.
|
OK, I've went through all these commits, and they look good. I had a remark about the style of the multiple The main issue is with the renaming - this appears to be the point we disagree on. I'll try to clarify my position here. I obviously want to avoid naming conflicts, and you want ergonomic names. But I don't think they need to be ergonomic. There are three reasons to have ergonomic names for types:
I don't see maintainability as an issue here, because this code is computer-generated. Humans are never supposed to edit it, and we'll never see these ugly symbols. Not unless we use Usability is more important - but the builder type is not going to be usable anyways. At least - not as a typed the users are going to name. They are so riddled with generic parameters that must accept special types that change with each setter call that I just don't see how being able to easily name them is going to be useful. Users are just going to rely on the fluent style and maybe type inference and avoid naming them altogether. That leaves us with documentation. Consider this simple struct: #[derive(typed_builder::TypedBuilder)]
#[builder(doc)]
pub struct Foo {
pub bar: u32,
pub baz: u32,
pub qux: u32,
}This creates a This is so messy, that it's going to be easier to read the unstructured docs of the |
|
I agree that the But that doesn’t mean that the builder type’s docs should be deliberately scuppered; there’s useful information in it. Notably, if the fields of the type to be built are not public, you can’t replace the true docs with an equivalent textual representation that will be quite as useful. Also if you add to the builder type—a case I’ll mention more below. The strong convention I have observed on Yet let’s explore an alternative purpose of The crux of this matter is then whether the typed builder type should be considered part of the public API. I believe it should be, and needs to be able to be. I want to be able to extend the builder with methods of my own to do things like setting multiple values at once, or perhaps to implement other traits on the builder type—I see typed-builder as a labour-saving device to generate most of what I might otherwise have written by hand by the sweat of my brow. Therefore, I want the output of the macro to be predictable, well-documented, and part of the versioning compatibility guarantee of the typed-builder crate. Defaulting to the name Let us also consider the implications of a public typed builder type being considered part of the public API. (This is definitely the safe approach to semver.) In that case, a change in the generics of that type is a backwards-incompatible change, and so adding new fields to the type, unless they are |
But this is my point- you can't. The builder type really is an implementation detail that even though the user can access should not be considered part of the API. Consider, for example, this extension to a builder type adds a method to set two fields at once: #[derive(typed_builder::TypedBuilder, PartialEq)]
struct Foo {
a: i32,
b: i32,
}
impl FooBuilder<(), ()> {
fn a_and_b(self, a: i32, b: i32) -> FooBuilder<(i32,), (i32,)> {
self.a(a).b(b)
}
}
fn main() {
assert!(Foo::builder().a(1).b(2).build() == Foo::builder().a_and_b(1, 2).build());
}With your branch this builds and runs successfully. But what if we wanted to add a new field to #[derive(typed_builder::TypedBuilder, PartialEq)]
struct Foo {
a: i32,
b: i32,
c: i32,
}
impl FooBuilder<(), ()> {
fn a_and_b(self, a: i32, b: i32) -> FooBuilder<(i32,), (i32,)> {
self.a(a).b(b)
}
}
fn main() {
assert!(Foo::builder().a(1).b(2).c(3).build() == Foo::builder().a_and_b(1, 2).c(3).build());
}This fails with impl<T> FooBuilder<(), (), T> {
fn a_and_b(self, a: i32, b: i32) -> FooBuilder<(i32,), (i32,), T> {
self.a(a).b(b)
}
}There are three problems with this:
Because of this, I feel like the users should consider the builder types as magical entities that they shouldn't touch directly - only call their setter and |
|
Then again - if we could somehow create helpers to smooth this... maybe some traits? Pack all the generics into a single tuple, define traits on that tuple, and then: impl<T> FooBuilder<T>
where T: FooBuilderTraits::NoA,
T: FooBuilderTraits::NoB
{
fn a_and_b(self, a: i32, b: i32) -> T::WithA::WithB {
self.a(a).b(b)
}
}The tricky parts are:
|
|
As I said, I consider it acceptable that adding a new non- I’ve tried playing round with the idea of yet more traits; I’m fairly sure that it’s not possible at present. https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=0a464f31edcdd36c5a12558c8ea61ec7 contains some of my experimentation. With tuples, you get stuck on the tricky point 1 that you mentioned, that It may be here that we reach an impasse and head amicably in different directions. But I would like it if we can resolve this to everyone’s satisfaction; even if we still differ on this key point, there is a feasible path forward for at least the immediate future where we support both philosophies with whatever attributes lead to the presence or absence of |
I can go with that. Let the user decide between allowing to extend the builder traits and making an addition of fields with Which brings us back to https://github.com/idanarye/rust-typed-builder/pull/16/files#r267600012. Now the inclusion or exclusion of attributes related to the builder type are no longer just about the doc, but about whether or not the builder type is part of the public API, I think it makes even more sense to nest them. But call it #[builder(builder_method_doc = "...", builder_type(doc = "...", build_method_doc = "..."))]Or - if you hate nesting that much - use another attribute: #[builder(builder_method_doc = "...")]
#[builder_type(doc = "...", build_method_doc = "...")] |
|
@chris-morgan - are we still doing this? |
|
FWIW I have been using this PR without any issues/bugs. Thanks for all the added features! One minor pain point that should be explained very prominently in the docs is that you can only refer to previously defined fields from field Also to add my two cents- I prefer nested attributes rather than flat with underscores. |
|
@mwilliammyers I'm worried less about bugs (I think the tests pretty much cover everything) and more about settling on a syntax. I'll give it another week, and if there is no progress in the discussion I'll just merge this and do the syntax changes I suggested before releasing a version. |
|
bikeshedding here—what do you guys think about |
|
TL;DR: I think customizing methods on the While I do agree with:
I have a good real-world case for supporting "customizing" builder methods for JWTs: use std::{error::Error, time::Duration};
use chrono::{DateTime, Duration as ChronoDuration, Utc};
use typed_builder::TypedBuilder;
// Ideally we would want the `issued_at` and `expires_at` fields to be excluded
// so we were forced to use the convenience `expires_in` method so we can ensure
// that you can only issue `JWTs` with an `issued_at` of now... but that isn't
// possible as of right now?
#[derive(TypedBuilder, Clone, PartialEq, Debug)]
struct Claims {
// #[builder(exclude, default)]
issued_at: DateTime<Utc>,
// #[builder(exclude, default_code = "issued_at + chrono::Duration::hours(1)")]
expires_at: DateTime<Utc>,
// snip the rest of the fields...
}
impl ClaimsBuilder<(), ()> {
/// Sets the `issued_at` and `expires_at` fields from a `Duration`.
fn expires_in(
self,
expires_in: Duration,
) -> Result<ClaimsBuilder<(DateTime<Utc>,), (DateTime<Utc>,)>, Box<dyn Error>> {
let issued_at = Utc::now();
let expires_at = issued_at + ChronoDuration::from_std(expires_in)?;
Ok(self.issued_at(issued_at).expires_at(expires_at))
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_expires_in() {
let expires_in = Duration::from_secs(600);
let actual = Claims::builder().expires_in(expires_in).unwrap().build();
// we would actually also have to create an `issued_at` to use this,
// but we cheat and use the existing one so we don't need to impl a custom `PartialEq`
let expected = Claims::builder()
.issued_at(actual.issued_at)
.expires_at(actual.issued_at + ChronoDuration::from_std(expires_in).unwrap())
.build();
assert_eq!(actual, expected);
}
}I find the |
I don't really care either way, as both make names are not entirely correct - we don't skip or exclude the field, as we still need to put values in them. I have no issue with following serde's style.
I was never concerned with this feature's usefulness - only with its usability. First, note that in your example you could just make let actual = Claims::builder().expires_in(expires_in).unwrap().build();Use: let actual = Claims::expires_in(expires_in).unwrap();Making it a builder method only makes sense if you have more fields: #[derive(TypedBuilder, Clone, PartialEq, Debug)]
struct Claims {
issued_at: DateTime<Utc>,
expires_at: DateTime<Utc>,
field_1: u32,
field_2: String,
field_3: Vec<u8>,
}Which complicates the definition: impl<T1, T2, T3> ClaimsBuilder<(), (), T1, T2, T3> {
fn expires_in(
self,
expires_in: Duration,
) -> Result<ClaimsBuilder<(DateTime<Utc>,), (DateTime<Utc>,), T1, T2, T3>, Box<dyn Error>> {
let issued_at = Utc::now();
let expires_at = issued_at + ChronoDuration::from_std(expires_in)?;
Ok(self.issued_at(issued_at).expires_at(expires_at))
}
}The more fiels you have - the more work you need to do... |
|
I'll need to experiment a bit to see if this actually works, but maybe if I put all the builder fields in a single tuple and have that tuple type as a single generic parameter for the builder type, I could generate traits for manipulating that tuple type: impl<F: ClaimsBuilderHasNoExpiresIn + ClaimsBuilderHasNoExpiresAt> ClaimsBuilder<F>
{
fn expires_in(
self,
expires_in: Duration,
) -> Result<
ClaimsBuilder<<<F as ClaimsBuilderHasNoExpiresIn>::WithIt as ClaimsBuilderHasNoExpiresAt>::WithIt>,
Box<dyn Error>
> {
let issued_at = Utc::now();
let expires_at = issued_at + ChronoDuration::from_std(expires_in)?;
Ok(self.issued_at(issued_at).expires_at(expires_at))
}
}Which is still unsightly - but now this hideosity can be hidden behind macros: impl<F: typed_builder::fulfills!(Claims; !expires_in, !expires_at)> ClaimsBuilder<F>
{
fn expires_in(
self,
expires_in: Duration,
) -> Result<
ClaimsBuilder<typed_struct::transform!(F: Claims; +expires_in, +expires_at)>,
Box<dyn Error>
> {
let issued_at = Utc::now();
let expires_at = issued_at + ChronoDuration::from_std(expires_in)?;
Ok(self.issued_at(issued_at).expires_at(expires_at))
}
} |
|
I should have been clearer - I have more fields in the struct. #[derive(TypedBuilder, Serialize, Deserialize, Clone, Debug)]
pub struct Claims {
#[serde(alias = "sub")]
subject: Id,
#[serde(alias = "iss")]
issuer: Url,
#[builder(default_code = "vec![issuer.clone()]")]
#[serde(alias = "aud")]
audience: Vec<Url>,
#[serde(alias = "iat", with = "serde_unix_timestamp")]
issued_at: DateTime<Utc>,
#[serde(alias = "exp", with = "serde_unix_timestamp")]
expires_at: DateTime<Utc>,
}I have a few more custom fields too- those are just most of the official JWT spec fields... |
Makes sense—we could simply add something small in the docs with an example similar to: impl<T1, T2, T3> ClaimsBuilder<(), (), T1, T2, T3> {
fn expires_in(
self,
expires_in: Duration,
) -> Result<ClaimsBuilder<(DateTime<Utc>,), (DateTime<Utc>,), T1, T2, T3>, Box<dyn Error>> {
let issued_at = Utc::now();
let expires_at = issued_at + ChronoDuration::from_std(expires_in)?;
Ok(self.issued_at(issued_at).expires_at(expires_at))
}
}and note that it is not really recommended or supported...? And we can add a note that it might not be wise to expose that method via a public API because adding fields (even if there is a default impl) is a breaking change to that method. Once I understood how the type params worked (by looking at the examples in this PR) it was really straightforward to |
|
But this is exactly what I'm trying to prevent - manual code with long lists of generic parameters that breaks as soon as you add or remove a field... If I can get my trait-based design to work, humans could use the names of the fields when they |
|
Yeah that would be ideal but for me manually implementing them as they are now isn't a dealbreaker. Would your trait-based system break backwards compatibility? I don't think it would? Would you be up for merging this (modulo changing I would be happy to open a new PR with all of these commits + any other blockers? |
|
Just playing devil's advocate here:
I am not 100% sold on the trait-based system being worth the effort? I echo previous sentiments and say let the users decide when to introduce a breaking change. All that being said, an argument in favor of the trait-based system is: limiting the scope/impact of breaking changes is always a good thing and so we should seek to limit it to the fields impacted by the custom builder if possible. |
I just need one small change to make it backward compatible - change the builder style from: struct FooBuilder<T1, T2, T3> {
f1: T1,
f2: T2,
f3: T3,
}To: struct FooBuilder<F> {
fields: F,
}Where (Note: I skipped the phantom data's fields, but the structs should have them too)
The same could be said about function names vs the long list of bytes representing their memory addresses. But at any rate - I plan to use helper macros anyway because the syntax for accessing associated types of types is ugly and cumbersome, and these macros can do the field name mangling as well.
But adding a field with a default shouldn't be a breaking change. Also, doing a breaking change doesn't mean you are OK with breaking every piece of code you ever written. Why should adding more fields to |
|
per my first point: Ahhhh so the end user would actually write their custom builder method using the macros and it would look something like your example: impl<F: typed_builder::fulfills!(Claims; !expires_in, !expires_at)> ClaimsBuilder<F>
{
fn expires_in(
self,
expires_in: Duration,
) -> Result<
ClaimsBuilder<typed_struct::transform!(F: Claims; +expires_in, +expires_at)>,
Box<dyn Error>
> {
let issued_at = Utc::now();
let expires_at = issued_at + ChronoDuration::from_std(expires_in)?;
Ok(self.issued_at(issued_at).expires_at(expires_at))
}
}If that is the case, I think my first point is essentially moot. I didn't catch that before... Bikeshed: maybe we should do Just to double check: that is more or less saying
100% agree, which is what I was trying to get at in the end of my comment. I was mostly playing devil's advocate just to make sure we hashed it all out. |
This adds various things that I want, and improves documentation ergonomics.
Look at it commit-by-commit rather than as one big diff.
The commit messages say what it’s all about, I shan’t repeat it here.
Fixes #13.
Arguably resolves #11.
One decision that I haven’t changed in this, but think should be changed:
#[doc = hidden]on the builder type. In this work I’ve made it possible to generate docs for it, with#[builder(doc)]on the type, but I haven’t made that the default. I think it should be, with#[builder(builder_type_doc = hidden)]then supported if you really want to hide it.I don’t think hiding docs for such a thing is a good idea. #11 hints at part of why: the types of the fields are not at all clear. The messy added generics are the main reason against it, and I’ve tidied that up just about as much as is possible in here, so that things are fully legible, but still without name collisions.
A further note on
()and(T,)and ergonomics and such: I planned something exactly like typed-builder a couple of years back; but at that time I only got as far as making what I called type-level Option. (Derive macros weren’t as mature as they are now, and I gave up part-way through the implementation of the actual macro.) Using()and(T,)didn’t occur to me. What I had wasTOption<T>as what you here call the conversion helper trait (with the most prominent methodinto_option, which you could then use as.into_option().unwrap_or(default)) andTSome<T> and TNone<T>. Using such names would increase verbosity from(T,)and(), but probably help with clarity, especially in the case ofTNone<Foo>versus(). The trouble with doing it that way, of course, is that proc-macro crates can’t export other things at present, so you’d end up with needing the user to addtoptionas well astyped-builder, which isn’t ideal. (I have that crate finished, but never published it, by the way.)P.S. you’ll note I’ve put myself in the authors list in Cargo.toml; if you’re willing, I’d like being a maintainer on this repository and on crates.io.