Skip to content
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

RFQ - Weigh in on whether the implicit operator (negating need for .Build()) is confusing or useful #60

Open
cottsak opened this issue Sep 23, 2016 · 5 comments
Labels

Comments

@cottsak
Copy link

cottsak commented Sep 23, 2016

I'm building a user like var antUser = new UserBuilder().WithUniqueName("ant"); and its failing a test. When I add the explicit build eg var antUser = new UserBuilder().WithUniqueName("ant").Build(); the test passes.

What I think is happening is a combination of two things:

  • The automagic value assignment of my Id field, and
  • The implicit operator which is calling BuildObject() internally

The failure is due to the same antUser being used twice in the test setup and I'm expecting the identity of said User to be the same with both usages of the instance, only it's not! The Id is different every time the implicit cast happens.

This is a pretty big deal and I think most devs would expect the identity/equality to behave the same way I am.

The implicit operator is misleading in that it allows the dev to think they don't really need to call .Build() explicitly (yay! saved banging a few keys) and so they don't. It's not obvious that the fix is to call .Build(). And then when you figure it out it seems smelly that you need to be aware of this peculiar detail.

Thoughts?

@mwhelan
Copy link
Member

mwhelan commented Sep 23, 2016

Hey @cottsak. Thanks for the comment.

The implicit operator is a bit controversial and Rob and I debated it a lot when I implemented it. There's a trade off between the convenience you mention and the desire to be quite explicit. I think it's clearly documented, which hopefully achieves the right balance.

Basically, if you use var to define your variable you have to call Build, otherwise how does the compiler know what to create? And, if you want to use the implicit creation you have to explicitly declare the variable.

But, given this particular scenario, I'm not quite sure how using the same antUser twice is working. If you are declaring it twice then I would expect two different instances with different Id values, but if you are declaring one variable and using it twice you'd obviously expect the same values... Do you have an example of the whole test?

@robdmoore
Copy link
Member

I can see how that would cause a problem, but I'm happy enough that it's discoverable when you realise that antUser is of type UserBuilder rather than User.

All of the code samples include an explciit .Build() call, it's only when you view http://dossier.teststack.net/docs/creating-entities-implicitly that we introduce the implicit operator as a thing.

I'm happy enough with that.

@cottsak
Copy link
Author

cottsak commented Jan 10, 2017

I find the implicit operator misleading. It's there and so folks will use it. The presence of the operator "reveals" a contract to the user (aided by productivity tools like R#, etc that go and find these things more proactively) whether the examples always use Build() or not.

Speaking to "documentation" @mwhelan, as I highlighted in this other issue, folks aren't going to go and read doco in the first instance and they may not even resort to the doco if something doesn't behave the way they expect. UX (which applies as much to a package/lib as much as a UI form) is predicated on conventions (among other things). Those conventions are expectations that users have in advance and implicitly apply to your interface. In this case, the explicit requirement to Build() every time would have been easier to understand and use as opposed to discovering an implicit operator and then it causing me more pain because it doesn't do what I expected.

In terms of ROI, how did you guys justify adding the implicit operator in your "debating" @robdmoore @mwhelan?

I like these tools and I want to promote them wherever I go provided it fits with my philosophy and I can advertise to other devs how easy they are. If I can understand your thinking then perhaps I can be more forgiving to these kinds of peculiarities.

@robdmoore
Copy link
Member

I personally don't use the implicit operator at all because I prefer the explicit .Build call and I also like var :P

One thing I will say though is that you should keep in mind that you are only but a single user Matt, other people's expectations may not match yours. I don't want to discount your opinion though since it's also possible that the majority of user's match your opinion.

I'd prefer if you tone down your language though mate - this is an OSS library, not something we get paid for so calling decisions that have been made "smelly" and "peculiarities" rather than giving benefit of the doubt is a little "unsettling" to use your phrase from the other thread...

If we have a few different users that also feel the implicit operator is confusing I don't object to taking it out, but I'd like to see a reasonable discussion about both sides and if we did it we'd obviously have to bump the major version since it's a breaking change.

@robdmoore robdmoore reopened this Jan 10, 2017
@robdmoore robdmoore changed the title Implicit Build() is causing me issues RFQ - Weigh in on whether the implicit operator (negating need for .Build()) is confusing or useful Jan 10, 2017
@cottsak
Copy link
Author

cottsak commented Jan 10, 2017

@robdmoore I'm a passionate guy as you know Rob and because I love this tool I can get direct when it fails to keep me perfectly satisfied. I also have a very narrow view of the world when it comes to writing great software and possibly because I know you a little Rob, I hold you (and by extension this package) to a higher standard than others. Perhaps I've done the wrong thing.

But you're right, I am only one user. Thanks for engaging me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants