-
Notifications
You must be signed in to change notification settings - Fork 18
Adds a Set overload which takes a lambda factory method #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- The specific use case for this is that we wanted to be able to use equivalence classes. However, if you just use them naively, it would generate a single value and use it for all of the instances it created. - The lambda overload not only enables this case, but gives some power to constructing incrementing counters and the like. For example, using a Queue to generate an ordered set of names, or other such use cases.
var builder = new CustomerBuilder() | ||
.Set(x => x.FirstName, "Pi") | ||
.Set(x => x.LastName, "Lanningham") | ||
.Set(x => x.YearJoined, () => counter++); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wanted to provide an overload that would take a lambda Func from TObj to TValue, so that you could base your value on the partially constructed object (i.e. make sure that AmountPaid was less than AmountOwed, or something),but that would have required too much internal structuring, so I left it for another day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be too hard to do the partially constructed object, you could do one that takes the builder so you can use a Get
call though?
The more I think about this, I kind of want to replace the values collection entirely. i.e., remove _properties, and when you Set with an explicit value, we just store () => value in the _propFactories. This makes things simpler, and works pretty well for some other ideas I have (and will be submitting a pull request for soon) What do you think? |
Agreed - I'd much prefer that. It's a much cleaner way of doing it. I'd still call it |
My coworker suggested that the lambda take the the AutoValueFixture, so you can write .Set(x => x.YearJoined, any => any.PositiveInteger()); What do you think of that idea as well? |
…hods - It's a lot cleaner to use the factory lambdas as just constant functions for the other Set method.
Yeah, that does make sense. I wonder how that will affect our ability to also pass in a builder... |
return Any.Get(type, propertyName); | ||
return _properties[propertyName]; | ||
Func<object> factory; | ||
if (_properties.TryGetValue(propertyName, out factory)) return factory(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be on two lines to meet the coding conventions of this project please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Do you have the coding conventions documented somewhere? I looked for a .md file somewhere and didn't see one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not documented no - just based on the rest of the code ;)
e.g. see the if statement in the deleted lines just above this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend starting a document with this case, and add to it in the future. It's hard to pick up on what the coding style rules of a project are just from the rest of the code, especially for a new contributor. If you want, I'll include it in my pull request.
What do you mean also pass in a builder? Did you mean, a builder for the second parameter? because that's as simple as
|
I more meant what we were talking about above about passing the builder instance (where you mentioned passing in a "partially constructed object") into the lambda |
Ah, right. I'll leave that in your hands to decide which you think would be more useful. We can always create an AutoValueFixture and use it via closure for that use case, |
- This will help new contributors get started, and keep them from stepping on anyone's stylistic toes. - Expand as you see fit!
Adds a Set overload which takes a lambda factory method
Thanks for this |
That's cool about the CONTRIBUTING.MD, I didn't know that! Awesome! And no worries! I plan on submitting another pull request in the next few days, but I'm going to open an issue first to let you provide your input on it. |
FYI there are a couple of minor things I need to look at then I'll push to NuGet. I'm doing a presentation at a virtual conference about this library next week so it will be then by the latest, but I hope to do it before then. |
Oh awesome, what conference? |
Thanks again for this - I've released it as v3.1.0 |
to use equivalence classes. However, if you just use them naively,
it would generate a single value and use it for all of the instances
it created.
to constructing incrementing counters and the like. For example,
using a Queue to generate an ordered set of names, or other such use
cases.