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.

Sign up for GitHub

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

Jump to bottom

Topic/typeconstraint allow string objects #169

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Topic/typeconstraint allow string objects #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c1aed17
Test that type constraint messages can be objects
jrubinator May 18, 2018
eb7aae9
Allow string objects in type constraint errors
jrubinator May 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion lib/Moose/Exception/ValidationFailedForInlineTypeConstraint.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,28 @@ use Moose;
extends 'Moose::Exception';
with 'Moose::Exception::Role::Class';

# An indiscriminate `use` without an import list somehow makes
# type_constraint_message a hash memory address
use Moose::Util::TypeConstraints qw(
duck_type

coerce
from
via

subtype
as
);
Copy link
Member

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.

subtype '_MooseImmediateStr' => as 'Str';
subtype '_MooseDucktypeStr' => as duck_type([qw< ("" >]);
Copy link
Member

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:

  1. We don't need to define a bunch of new type names for this, just use anon types.
  2. It's clearer to use overload::Method to figure out if something is stringifiable.
  3. 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?

Copy link
Member

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.

Copy link
Author

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?

Copy link
Member

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.

coerce '_MooseImmediateStr',
from '_MooseDucktypeStr',
via { "$_" };

has 'type_constraint_message' => (
is => 'ro',
isa => 'Str',
isa => '_MooseImmediateStr',
coerce => 1,
required => 1
);

Expand Down
53 changes: 53 additions & 0 deletions t/bugs/stringifiable_type_constraint_messages.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
use strict;
use warnings;

# RT 123299

use Test::More;
use Test::Fatal;

{
package String::Object;

use overload '""' => \&as_string, fallback => 1;

sub new {
my $self = bless {}, __PACKAGE__;
}

sub as_string { 'successful exception' }

package Bad::Moose::Types;

use Moose;
use MooseX::Types::Moose qw( Str );
Copy link
Member

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.

use MooseX::Types -declare => [ 'AlwaysFail', 'StringObject' ];

subtype AlwaysFail,
as Str,
where { 0 },
message { String::Object->new };

class_type StringObject, { class => 'String::Object' };
coerce Str, from StringObject, via { 'coerced' };

package Bad::Moose;
use Moose;

has validation_fail => (
is => 'ro',
isa => Bad::Moose::Types::AlwaysFail,
);

__PACKAGE__->meta->make_immutable; # This is the key line
}

package main;

like(
exception { Bad::Moose->new( validation_fail => '123' ) },
qr/successful exception/,
"Moose type constraints accept stringifiable type constraint errors"
Copy link
Member

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

);

done_testing();