Mojo::Base::with_roles patch #1120

dotandimet · 2017-08-09T16:56:19Z

Summary

Add with_roles method to Mojo::Base, per the specs described here:
https://irclog.perlgeek.de/mojo/2017-08-08#i_14984146

Motivation

Allow multiple developers to extend key Mojolicious classes (Test::Mojo, Mojo::UserAgent, etc)
using Role::Tiny (optional dependency)

References

https://irclog.perlgeek.de/mojo/2017-08-08#i_14984146

jberger · 2017-08-09T17:03:58Z

lib/Mojo/Base.pm

@@ -61,6 +61,10 @@ sub attr {
  }
 }

+sub can_roles {
+  return !!(eval 'require Role::Tiny' && $Role::Tiny::VERSION >= 2.000005);


This should be a constant and the can_roles method is just for exposure outside of the class. C.f. https://github.com/kraih/mojo/blob/be50b20ae6c80df78a2594fdd198c19364f649ad/lib/Mojo/IOLoop/Client.pm#L13-L15

jberger · 2017-08-09T17:05:31Z

lib/Mojo/Base.pm

@@ -101,6 +105,16 @@ sub tap {
  return $self;
 }

+sub with_roles {
+  if ($_[0]->can_roles()) {


Within the class, it should use the constant (see above), since this will be constant folded at compile time and not introduce overhead on each with_roles call

kraih · 2017-08-09T17:08:31Z

lib/Mojo/Base.pm

@@ -61,6 +61,10 @@ sub attr {
  }
 }

+sub can_roles {
+  return !!(eval 'require Role::Tiny' && $Role::Tiny::VERSION >= 2.000005);
+}


This is not consistent with our other optional dependencies.

kraih · 2017-08-09T17:09:01Z

lib/Mojo/Base.pm

@@ -101,6 +105,16 @@ sub tap {
  return $self;
 }

+sub with_roles {
+  if ($_[0]->can_roles()) {
+    Carp::croak "class and roles must be package names" if !!grep { ref $_ } @_;


Is there a good reason that argument validation is required here?

create_class_with_roles will error if anything other than a valid role name is passed, so probably not needed

It may be worthwhile checking that the invocant is not an object though, there's no explicit check for that in Role::Tiny and it could lead to weird things being loaded based on object stringification

I'll change it to only check the invocant.

That's not something we normally do.

I'll remove it and see how cryptic the error is in the tests...

OK, my error is nice, but if you try to invoke with_roles on an object, you get a terrifying exception, so I'm fine with that (something like "Can't locate Mojo/RoleTest/Base__WITH__Mojo/RoleTest/LOUD=HASH(0x17e56b0).pm in @inc", or whatever)

kraih · 2017-08-09T17:09:43Z

lib/Mojo/Base.pm

+    return Role::Tiny->create_class_with_roles(@_);
+  }
+  else {
+    Carp::croak "Role::Tiny 2.000005 is required for with_roles method";


Do we really require 2.000005, or would a nicer looking 2.0 work too?

I would recommend at least 2.000001 for an important fix to created class names. https://rt.cpan.org/Public/Bug/Display.html?id=103310

I'll change it to 2.000001+

kraih · 2017-08-09T17:10:18Z

lib/Mojo/Base.pm

+
+  my $roles_ok = Mojo::Base->can_roles()
+
+True if L<Role::Tiny> 2.000005+ is installed and roles are supported in Mojo::Base


Mojo::Base is not a link.

kraih · 2017-08-09T17:10:47Z

lib/Mojo/Base.pm

@@ -210,6 +224,13 @@ executed at accessor read time if there's no set value, and gets passed the
 current instance of the object as first argument. Accessors can be chained, that
 means they return their invocant when they are called with an argument.

+=head2 can_roles
+
+  my $roles_ok = Mojo::Base->can_roles()


We write my $bool = ... for methods that return booleans.

Also missing semicolon.

kraih · 2017-08-09T17:12:17Z

lib/Mojolicious/Command/version.pm

@@ -27,6 +28,7 @@ OPTIONAL
  IO::Socket::Socks 0.64+ ($socks)
  IO::Socket::SSL 1.94+   ($tls)
  Net::DNS::Native 0.15+  ($nnr)
+  Role::Tiny 2.000005+ ($roles)


Indentation.

kraih · 2017-08-09T17:13:22Z

t/mojo/roles.t

+use Mojo::Base -strict;
+use Test::More;
+
+plan skip_all => "Role::Tiny required for roles" unless Mojo::Base->can_roles;


This is inconsistent with other tests for optional dependencies.

kraih · 2017-08-09T17:14:13Z

t/mojo/roles.t

+
+# First test derived from sri's comment here:
+# https://github.com/kraih/mojo/pull/1118#issuecomment-320930345
+# TODO: make this a subtest


We don't use these kinda comments.

kraih · 2017-08-09T17:14:43Z

t/mojo/roles.t

+package Test::Mojo::Role::Location;
+
+use Role::Tiny;
+use Test::More; # for is()


Used twice.

kraih · 2017-08-09T17:15:06Z

t/mojo/roles.t

+$t->get_ok('http://mojolicio.us')
+  ->status_is(301)
+  ->location_is('http://mojolicious.org/')
+  ->or(sub { diag 'doh!' });


I don't think this is a good test case, keep it simple.

kraih · 2017-08-09T17:32:17Z

t/mojo/roles.t

+
+use Test::Mojo;
+
+plan tests => 3;


We don't use plans.

kraih · 2017-08-09T17:33:07Z

t/mojo/roles.t

+plan tests => 3;
+
+my $t = Test::Mojo->with_roles('Test::Mojo::Role::Location')->new();
+$t->get_ok('http://mojolicio.us')


Tests shouldn't require an internet connection.

kraih · 2017-08-10T14:15:55Z

Don't forget to squash your commits.

aferreira · 2017-08-10T15:01:58Z

lib/Mojo/Base.pm

@@ -15,6 +15,10 @@ use IO::Handle ();
 my $NAME
  = eval { require Sub::Util; Sub::Util->can('set_subname') } || sub { $_[1] };

+# Role support requires Role::Tiny 2.000001+
+use constant ROLES =>
+  !!(eval 'require Role::Tiny' && $Role::Tiny::VERSION >= 2.000001);


why not eval 'use Role::Tiny 2.000001 (); 1';
like at https://github.com/kraih/mojo/blob/master/lib/Mojo/IOLoop/TLS.pm#L11 ?

Actually the best method would be eval { require Role::Tiny; Role::Tiny->VERSION('2.000001'); 1 }

String eval is unnecessary for this, and the ->VERSION method (same as used by use) is the correct way to check a module's version.

That does look much better.

aferreira · 2017-08-10T15:02:36Z

lib/Mojolicious/Command/version.pm

@@ -27,6 +28,7 @@ OPTIONAL
  IO::Socket::Socks 0.64+ ($socks)
  IO::Socket::SSL 1.94+   ($tls)
  Net::DNS::Native 0.15+  ($nnr)
+  Role::Tiny 2.000005+ ($roles)


kraih · 2017-08-10T16:21:22Z

t/mojo/roles.t

+
+BEGIN {
+# bail in a BEGIN block, so we can use Role::Tiny in inline classes below
+  use Test::More;


Move the use Test::More outside the block and remove the comment.

kraih · 2017-08-10T16:22:14Z

lib/Mojolicious/Command/version.pm

@@ -27,6 +28,7 @@ OPTIONAL
  IO::Socket::Socks 0.64+ ($socks)
  IO::Socket::SSL 1.94+   ($tls)
  Net::DNS::Native 0.15+  ($nnr)
+  Role::Tiny 2.000001+ ($roles)


Indentation is still wrong.

Support extending Mojo::Base classes with roles, using Role::Tiny (optional dependency)

jberger reviewed Aug 9, 2017

View reviewed changes

kraih requested changes Aug 9, 2017

View reviewed changes

kraih reviewed Aug 9, 2017

View reviewed changes

t/mojo/roles.t Outdated

use Test::Mojo;

plan tests => 3;

Copy link

Member

kraih Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use plans.

kraih reviewed Aug 9, 2017

View reviewed changes

kraih mentioned this pull request Aug 10, 2017

Add 'Mojolicious::Guides::Testing' tutorial. Resolves #1111 #1118

Merged

aferreira reviewed Aug 10, 2017

View reviewed changes

kraih reviewed Aug 10, 2017

View reviewed changes

dotandimet force-pushed the with_roles branch 2 times, most recently from eb3cca3 to 4700660 Compare August 10, 2017 16:45

Mojo::Base::with_roles patch

c213395

Support extending Mojo::Base classes with roles, using Role::Tiny (optional dependency)

dotandimet force-pushed the with_roles branch from 4700660 to c213395 Compare August 10, 2017 16:47

kraih merged commit 492e6c4 into mojolicious:master Aug 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mojo::Base::with_roles patch #1120

Mojo::Base::with_roles patch #1120

dotandimet commented Aug 9, 2017

jberger Aug 9, 2017

jberger Aug 9, 2017

kraih Aug 9, 2017

kraih Aug 9, 2017

Grinnz Aug 9, 2017

Grinnz Aug 9, 2017 •

edited

dotandimet Aug 10, 2017

kraih Aug 10, 2017 •

edited

dotandimet Aug 10, 2017

dotandimet Aug 10, 2017

kraih Aug 9, 2017

Grinnz Aug 9, 2017

dotandimet Aug 10, 2017

kraih Aug 9, 2017

kraih Aug 9, 2017

kraih Aug 9, 2017

kraih Aug 9, 2017

kraih Aug 9, 2017

kraih Aug 9, 2017

kraih Aug 9, 2017

kraih Aug 9, 2017

kraih Aug 9, 2017

kraih Aug 9, 2017

kraih commented Aug 10, 2017

aferreira Aug 10, 2017

Grinnz Aug 10, 2017

kraih Aug 10, 2017

aferreira Aug 10, 2017

kraih Aug 10, 2017

kraih Aug 10, 2017


		my $roles_ok = Mojo::Base->can_roles()

		True if L<Role::Tiny> 2.000005+ is installed and roles are supported in Mojo::Base

Mojo::Base::with_roles patch #1120

Mojo::Base::with_roles patch #1120

Conversation

dotandimet commented Aug 9, 2017

Summary

Motivation

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Grinnz Aug 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraih Aug 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraih commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Grinnz Aug 9, 2017 •

edited

kraih Aug 10, 2017 •

edited