Closed pwil3058 closed 4 years ago
This sounds like a bug in your implementation of the macro? The invocation looks like it should work to me. There is only one env field.
I think the test expectations could be better documented. When the "each" name is the same as the field name (as is the case with "env" here), it's not possible to generate both a one-at-a-time setter AND an all-at-once setter.
The correct result could be any of the following:
If you meant for the name collision to be allowed, then (4) is no good. (3) seems at odds with the programmer's attempt to add something. (1) is maybe too limiting, and (2) seems like a weird side-effect-at-a-distance ("I added B, why did A go away?").
I think @jonhoo found this confusing during his stream also; he went with (2).
Ah, makes sense. I think I had 1 in mind in the reference implementation but 2 would be fine too. I would accept a PR to clarify this in the test case. Thanks!
@dtolnay thank you very much for this amazing workshop! Great job and it's being totally amazing for me!
Aside from the above clarification, I think there's still a bug or I didn't implement as desired since past exercises. The builder is called like this:
let command = Command::builder()
.executable("cargo".to_owned())
.arg("build".to_owned())
.arg("--release".to_owned())
.build()
.unwrap();
We are not calling the builder method to instantiate env
at any moment. So my macro is throwing field "env" is missing
since we are attempting to build()
and env is not an optional field - which I believe is behaving correctly. Did I miss anything?
@leordev: I would expect that on something like #[builder(each = "arg")] args: Vec<String>
, if .arg(...)
is called 2 times then args
gets built as a vector containing 2 elements; if .arg(...)
is called 0 times then args
gets built as a vector containing 0 elements.
Line 35 and 36 cause a "duplicate definitions with name 'env'" error.