Open GoogleCodeExporter opened 8 years ago
Hi tscott,
Gareth and I will look into this and get back to you soon.
Thanks for submitting a patch with your request. I love it when people do that
because it makes my job so much simpler.
Regards,
Joshua Roth
Original comment by joshuajr...@gmail.com
on 10 Sep 2010 at 12:51
Hi tscott,
Are you able to supply a copy of the DDDStyleClass you used? the class
declaration is missing from your patch... :-)
Thanks,
Josh
Original comment by joshuajr...@gmail.com
on 11 Sep 2010 at 1:48
[deleted comment]
Whoops. In VS2008, VisualSVN adds new files for me. Not in 2010 tho. Please
find corrected patch attached.
Original comment by timtast...@gmail.com
on 12 Sep 2010 at 6:46
Attachments:
Hi Tim,
(I tried sending this to your lunaversesoftware.com address but it bounced for
some reason so I'm forced to put my comments against this issue)
With regard to your updated patch for issue 50, I’ve attempted to integrate
it into the codebase but unfortunately I’ve run into some trouble.
When I run all the unit and functional tests, 82 of the tests now fail. The
failure reasons are grouped into two types of failure as listed below.
1)
Test
'FizzWare.NBuilder.Tests.Integration.ListBuilderTests_WithAClassThatHasAParamete
rlessConstructor.PropertiesShouldBeSetSequentially' failed:
FizzWare.NBuilder.BuilderException : The type requires constructor args but they have not be supplied for all the elements of the list
Implementation\ListBuilder.cs(59,0): at FizzWare.NBuilder.Implementation.ListBuilder`1.Construct()
Implementation\ListBuilder.cs(83,0): at FizzWare.NBuilder.Implementation.ListBuilder`1.Build()
Integration\ListBuilderTests_WithAClassThatHasAParameterlessConstructor.cs(33,0): at FizzWare.NBuilder.Tests.Integration.ListBuilderTests_WithAClassThatHasAParameterlessConstructor.PropertiesShouldBeSetSequentially()
2)
Test
'FizzWare.NBuilder.FunctionalTests.Extensibility.ExtensibilityTests.SpecifyingAC
ustomPropertyNamerForASpecificType' failed:
FizzWare.NBuilder.BuilderException : The type requires constructor args but they have not be supplied for all the elements of the list
Implementation\ListBuilder.cs(59,0): at FizzWare.NBuilder.Implementation.ListBuilder`1.Construct()
Implementation\ListBuilder.cs(83,0): at FizzWare.NBuilder.Implementation.ListBuilder`1.Build()
Extensibility\ExtensibilityTests.cs(50,0): at FizzWare.NBuilder.FunctionalTests.Extensibility.ExtensibilityTests.SpecifyingACustomPropertyNamerForASpecificType()
Did all the tests pass when you ran them for your patch? If so, is it possible
that your patch is missing something? If not, are you able to see if there is a
quick fix to this problem?
Hopefully I’ll be able to spend some time investigating why this patch is
causing issues, however I thought I’d notify you of the problem just in case
you are able to investigate in parallel.
Also, with the semantics of WithPrivate, I was thinking of investigating if it
would be possible to just use the existing With semantics in order to keep
things simple. I haven’t had a chance to see if my suggestion makes any
sense, but what do you think?
Finally, I’ve been thinking of the name DddStyleClass and have been trying to
figure out a better name... It seems to me, that what this patch is really
about is allowing NBuilder to be able to build immutable/semi-imutable classes
with the same With semantics one would normally use for mutable classes. As
such, perhaps we should rename the class from DddStyleClass to ImmutableClass.
What do you reckon?
Regards,
Joshua Roth
Original comment by joshuajr...@gmail.com
on 13 Sep 2010 at 3:57
Gee, sorry about that. Shoddy work on my part. Certainly I ran all the tests
before submitting the patch, yes? Uh, apparently not. Please accept this
corrected patch.
You asked about using normal With semantics. I think that I chose WithPrivate
to call out the different signature:
With(x => x.Bar = "bar")
WithPrivate(x => x.Bar, "bar")
Otherwise it might be hard to understand why the signature is different. But I
don't feel strongly one way or the other.
Regarding ImmutableClass vs. DddStyleClass name. I don't have a strong
opinion. Notice that the example class is not actually immutable. I threw in
a state changing method just to illustrate the pattern -- behavioral business
entity versus anemic data structure entity. This is the need that I have and
that others probably have. That said, the changes I made certainly allow
NBuilder to support building of immutable objects.
Hopefully third time is a charm. Thanks!
Original comment by timtast...@gmail.com
on 13 Sep 2010 at 5:08
Attachments:
Hi Tim,
I've taken your patch and extended it a bit. You can see my changes at revision
69 and revision 70.
I have currently gone with the With semantics. This allows people to create
objects by .With(x => x.Bar, "bar").
Additionally I have added the ability to use the And semantics as well as
follows:
.With(x => x.Foo, "foo")
.And(x => x.Bar, "bar")
I have tried to mitigate confusion with the similar With and And signatures by
putting documentation on the SingleObjectBuilderExtensions class' methods. This
should be visible via Intellisense.
Also, in addition to being able to construct classes that have a private
parameterless constructor with private set properties, I have included support
for setting public readonly fields as well in the interest of completeness.
This was all made possible by the use of
(T)FormatterServices.GetUninitializedObject(typeof(T)) which is able to create
a given type without calling it's constructor. I'm hoping this won't have any
unforeseen consequences for the objects people construct.
Let me know if you're happy with the changes I've made and if you can see any
potential issues with my solution.
Original comment by joshuajr...@gmail.com
on 14 Sep 2010 at 7:13
Original comment by joshuajr...@gmail.com
on 14 Sep 2010 at 7:57
Original comment by joshuajr...@gmail.com
on 14 Sep 2010 at 8:00
Just realised that we hadn't thought about adding support for constructing
lists of immutable classes. As such I've added support for the Have, Has, and
And methods in revision 73. Let me know if you're happy with this change.
Original comment by joshuajr...@gmail.com
on 14 Sep 2010 at 11:52
Looks fine to me. Normally objects I construct for testing always have some
parameterless constructor (the ORM needs it), but I can see no reason not to
bypass all constructors if the user so desires. You should be able to coerce
any object into any state you want!
Thanks a million for jumping on this! By the way, in case you don't know, I'm
the guy who contributed the RandomValuePropertyNamer way back when.
Original comment by timtast...@gmail.com
on 14 Sep 2010 at 1:11
FYI: http://tech.groups.yahoo.com/group/altdotnet/message/24179
Original comment by timtast...@gmail.com
on 14 Sep 2010 at 1:55
Hey Tim,
Thanks for your promoting NBuilder amongst the Alt.Net crowd. We do want this
tool to benefit as may developers as possible. :-)
I'm fairly new to the project but I did see that you have contributed to
components in the past. This is very much appreciated! I definitely appreciate
your submitting patches because it gives me a much better idea of what you're
after. The idea you put forward was also suggested in issue 44 but it was your
patch that helped me get started since I try to prioritise issues with supplied
patches over those that don't.
I'll be pushing for another release (probably a beta first) so we can get this
feature out into the wild. If this feature works out, I'd like to be able to
get it into developer's hands asap so people don't have to go building things
from the trunk. In my experience, Dev's usually prefer to work with proper
supported releases rather than ad-hoc trunk builds.
I'm looking forward to feature requests and patches from you in the future. Do
keep them coming. I'd really like to make this project the best test data
builder in all of .NET. :-)
Regards,
Joshua Roth
Original comment by joshuajr...@gmail.com
on 15 Sep 2010 at 7:47
I've made an additional change at revision 76. The change removes the use of
FormatterServices.GetUnititializedObject as this was causing compile issues in
the Silverlight version of NBuilder. The change doesn't impact too much on this
enhancement.
You can still construct classes of the following design:
public class MyClass()
{
private MyClass(){}
}
You just can't construct classes of the following design:
public class MyClass()
{
int a;
private MyClass(int a)
{
this.a = a;
}
}
The main reason is that (T)Activator.CreateInstance(typeof(T), true) creates
instances using the default parameterless constructor and thus cannot be used,
as far as my current understanding goes, to construct a class that someone has
gone to a lot of trouble to ensure that it cannot be constructed directly.
Shouldn't affect the original request though, but I thought you might like to
be kept up-to-date.
Original comment by joshuajr...@gmail.com
on 27 Sep 2010 at 12:33
Hey Gareth,
Can you please verify that you're happy for this change to go into the release.
Thanks,
Josh
Original comment by joshuajr...@gmail.com
on 21 Oct 2010 at 11:50
Original issue reported on code.google.com by
timtast...@gmail.com
on 10 Sep 2010 at 2:59Attachments: