-
Notifications
You must be signed in to change notification settings - Fork 105
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
Topic/typeconstraint allow string objects #169
base: master
Are you sure you want to change the base?
Topic/typeconstraint allow string objects #169
Conversation
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.
Thanks for working on this. I took a look and I had some feedback.
package Bad::Moose::Types; | ||
|
||
use Moose; | ||
use MooseX::Types::Moose qw( Str ); |
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.
We can't use MX::Types in Moose tests except as an optional dep, but I don't think you need to use MX::Types for this anyway.
|
||
subtype | ||
as | ||
); |
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.
This is very pretty, but we don't do this anywhere else in the code base, so I think it's better to just format this like everything else.
like( | ||
exception { Bad::Moose->new( validation_fail => '123' ) }, | ||
qr/successful exception/, | ||
"Moose type constraints accept stringifiable type constraint errors" |
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.
This is a little confusing. I think this would be better as Can use a stringifiable object as a type constraint failure message
as | ||
); | ||
subtype '_MooseImmediateStr' => as 'Str'; | ||
subtype '_MooseDucktypeStr' => as duck_type([qw< ("" >]); |
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's a bit more verbose but I think it's better to write this as something like this:
my $stringifiable = subtype(
as 'Str',
inline_as { qq[Scalar::Util::blessed( $_[0] ) && defined overload::Method( $_[0], q{""} )] },
);
So there's a couple things there:
- We don't need to define a bunch of new type names for this, just use anon types.
- It's clearer to use
overload::Method
to figure out if something is stringifiable. - The type should be inlineable for speed.
Note that you also use anon empty subtype of Str
for the coercion. But then I'm wondering why bother with the coercion at all. Is there any reason not to pass the object through as-is?
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.
Then I realized I'm the one who suggested coercing to a string in the first place. But was that a good suggestion? I'm not sure. I can see arguments both ways.
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.
Sorry for the delay on this (I was in Yosemite for at least part of the delay!) - I can see the argument for both as well. I personally would prefer to just pass the object through, but I think we're worried about backwards compatibility, correct?
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.
@karenetheridge - what are your thoughts here? I would argue that returning a stringifiable object is fine as a change for a major release.
e79a7d8
to
d45b381
Compare
Fix for RT#123299