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

Ball and Chain: toSingleBox now sets up the failure chain properly. #1609

Merged
merged 1 commit into from
Dec 14, 2014

Conversation

Shadowfiend
Copy link
Member

The ParamFailure that toSingleBox returns when there are Failures in the list
now has a chain of Failures on it that correspond to all of the Failures in the
list.

The ParamFailure that toSingleBox returns when there are Failures in the list
now has a chain of Failures on it that correspond to all of the Failures in the
list.
@farmdawgnation
Copy link
Member

Hmmm. So, I'm going to be honest. I'm not in love with this. Mostly because I always understood the semantic meaning of the chain parameter to have a causal relationship to the current failure. This is true in the case of the topmost ParamFailure, but not true of the first Failure whose chain is an earlier Failure in the List.

Do you see what I mean?

One possible iteration from this point is to have the topmost ParamFailure's chain be a ParamFailure[List[Failure]]. Thoughts?

@Shadowfiend
Copy link
Member Author

I do see what you mean, but I don't think it's a problem. I'll think about it some more. Making the param the list of failures isn't terrible, but if another failure in turn is wrapped around the param, you lose the original list and that means you've lost information about what you were trying to do. Not the end of the world, but if we can represent both I think it's a win.

@Shadowfiend
Copy link
Member Author

So yeah, I've thought about this some more. It's not the cleanest possible implementation, I agree, but using the param in a ParamFailure runs into type erasure issues (to properly extract it, you use a pattern match, and to properly pattern match the type, you'll have to pattern match on : List[Failure], and that causes erasure issues). So I think the API usability of the chain wins out over the ParamFailure for something that I think will be common if, say, people use this for form validation.

@farmdawgnation
Copy link
Member

< headscratch >

Yeah, I see what you mean.

Do you think a subclass of ParamFailure that is returned from toSingleBox would be a net-win? For example, CompositeFailure such that:

case class CompositeFailure[T](message: String, failures: List[Failure], param: T) extends ParamFailure[T]

The question here is, I guess, "Do we gain any API niceness by adding to the class hierarchy?"

@farmdawgnation
Copy link
Member

It occurs to me though that we're at the point where this conversation might should be happening on the ML...

@Shadowfiend
Copy link
Member Author

Yeah, you're probably right.

@Shadowfiend
Copy link
Member Author

Sent a summary to the list.

@farmdawgnation
Copy link
Member

🎊

@Shadowfiend
Copy link
Member Author

We didn't really reach a conclusion on this on the mailing list. Thoughts on what to do next? Shall I revive the thread?

@farmdawgnation
Copy link
Member

Yeah, let's revive the thread. If nobody else objects I'm fine with including it. My objection is primarily over semantic meaning, and it's probably nitpicking for perfection in an imperfect situation. ;)

@Shadowfiend
Copy link
Member Author

No further feedback was had, so I guess let's do it?

@farmdawgnation
Copy link
Member

I'm down.

farmdawgnation added a commit that referenced this pull request Dec 14, 2014
Ball and Chain: toSingleBox now sets up the failure chain properly.
@farmdawgnation farmdawgnation merged commit efc7976 into master Dec 14, 2014
@farmdawgnation farmdawgnation deleted the ball-and-chain branch December 14, 2014 20:56
@fmpwizard fmpwizard added this to the 3.0-M3 milestone Dec 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants