Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added Accessor property to ConvertProblem class. #50

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

emiaj commented Aug 10, 2012

This will allow to consumers to know the exact path in the chain of properties (nested/collections) of a given problem.
Updated code accordingly.

Contributor

emiaj commented Aug 10, 2012

The commit does not have unit tests just yet, first I would like to know if the code looks fine to you guys.
If tests are something you strongly require, I can add them tomorrow.

Member

ahjohannessen commented Aug 10, 2012

  1. AccessorNode is a bit weird named.

  2. What is that Serializable change for?

  3. Tests are needed for such crucial/complex functionality. Also they showcase what you intend to solve with this.

  4. Below seems too wacky IMO, why are you having both?

_logger = logger is AccessorLogger ? logger : new AccessorLogger(logger);

For some more solid feedback you'll need wait for @jeremydmiller :)

Contributor

emiaj commented Aug 10, 2012

  1. There is a test that dictates that ConvertProblem must be serializable.
    By that extend, any member of it should be serializable as well.
    That means, the Accessor and IValueGetter implementations must be serializable.
    https://github.com/DarthFubuMVC/fubucore/blob/master/src/FubuCore.Testing/Binding/BindingResultTester.cs#L49

emiaj added some commits Aug 9, 2012

Added Accessor property to ConvertProblem class.
This will allow to consumers to know the exact path in the chain of properties (nested/collections) of a given problem.
Updated code accordingly.
Increased coverage on BindingContext, in general, the code related to…
… the problem accessor.

Check: when_binding_a_child_object_that_has_some_invalid_data.
Contributor

emiaj commented Oct 13, 2012

I have increased a little bit the test coverage, gonna try to find some window of time to increase it even more.

Owner

jeremydmiller commented Nov 16, 2012

Jaime,

I've looked at this a bit this morning, and honestly, I think I'm voting against this change. All the information we need is already on the ConvertProblem (Item + Property gets you to it with some trickery). At this point, I think we can deal with this in FubuValidation / FubuMVC.Validation. FubuMVC.Validation is coming up on my radar pretty soon, maybe next Friday.

--Jeremy

@emiaj emiaj closed this Feb 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment