Role syntax is backwards compared to everything else

[shadowcat-mst]

with_roles now takes

'Fully::Qualified::Name'

or

'+RelativeName'

whereas Catalyst, MooseX::Traits, and basically every other implementation of a similar thing does it the other way around.

Suggestion: since the 'usual' way is, admittedly, somewhat confusing, instead accept

'Fully::Qualified::Name'
'::RelativeName'

since the :: should make the relative one obviously relative, and then '+' can either be removed entirely or become a no-op for compatibility with code that expects to be able to pass '+Fully::Qualified::Name' for a role/trait.

[shadowcat-mst]

Note that e.g. Mojolicious::Plugin defaults to accepting a RelativeName as well, so this is effectively backwards wrt that as well. This doesn't seem like a feature to me.

[kraih]

Catalyst is of no concern to us.

[kraih]

And regarding Mojolicious::Plugins, that syntax doesn't work for with_roles, so there is no reason for consistency.

[kraih]

Personally, i don't particularly care about which syntax to choose. I committed something that works and asked for feedback on IRC. Nobody objected so it got released.

[kraih]

If you're serious about changes, please open a pull request and we can see if the proposal gets the required votes.

[kraih]

For the record, i picked the +Foo version because it looked compatible with Test::Mojo::WithRoles. And i did get it backwards, that was unintended.

[shadowcat-mst]

Right, Test::Mojo::WithRoles is designed to be compatible with, well, everything-else-that-uses-roles.

I only mention the Catalyst design because (a) it was your design (b) it's what everything else has then copied from.

Do you want a PR that un-backwards-es it? That should be trivial.

[kraih]

We can't just switch it around, the deprecation policy applies.

[shadowcat-mst]

Then I'm not sure which pull request you were asking me to open?

[kraih]

One that includes a deprecation plan. 😉

[shadowcat-mst]

Hmm. So, what we could do is:

• Introduce ::Foo for relative (i.e. alias-to-current-+Foo)
• Make +Foo warn that it needs to be ::Foo
• Wait for a while for users to move
• Switch +Foo to mean absolute
• Make Foo warn that it needs to be +Foo
• Wait for a while
• Remove warning for Foo on its own so people can start to use that instead of ::Foo

Bit messy but it'd work.

Alternatively, lots of things like calling this method with_traits, so you could introduce with_traits with the finally-planned behaviour, and make with_roles warn that it's going to become an alias to with_traits at a certain point.

Also a bit messy but would also work.

Either of those sound worth starting? (also shall we add support for '+MyApp::Plugin::Foo' to Mojolicious while we're here?)

[kraih]

Everybody seems to like the current syntax. I guess the only reason to pick something else would be consistency with other modules. Perhaps that's not a good enough reason for messy deprecation periods.

[shadowcat-mst]

Having a situation where the two syntaxes are exactly inverted seems like the worst possible world to me.

What about just replacing + with something else? ~ or - would both work, the ~ implying 'mine' and the - because, well, ->with_roles(-Foo, -Bar) would be kinda neat. or '::' which I like because package syntax, and ->with_roles(qw(::Foo ::Bar)) looks very obvious to me. Though "not +" is the bit I care about, and am happy to defer to somebody else's aesthetics other than that.

Having only just got fedora convinced to make 'perl' actually perl again rather than having a perl/perl-core split that's 'exactly debian's except backwards', I'm possibly a little sensitive at the moment to the pain of "there are two APIs and they are a mirror image and good luck picking the right one" - I'm getting flashbacks to writing QBASIC at high school and BBC basic at home (fscking COLO(U)R command ;)

[marcusramberg]

I'm 👎 on this deprecation plan. Sounds like it introduces a lot more confusion and pain than it relieves.

[shadowcat-mst]

Well, yeah, getting out of the current situation to somewhere more sensible is going to be annoying.

But if you're talking about 'confusion and pain', consider that anybody porting Test::Mojo::WithRoles code or working on Mojo/Moo(se) combined code is going to find Mojo's with_roles() behaviour exactly backwards compared to the prior art. Which I suspect is going to be non-hilarious for users for a lot longer than the short term pain involved with tweaking the feature somehow - and sri did say that getting it backwards wasn't actually deliberate ...

[latk]

I personally find the Absolute, +Relative syntax far more intuitive: if something looks like a package name, it should be a package name with no magic applied. The + clearly suggests that something is added to the name.

That would have been great in a vacuum. But given the overwhelming prior art by other modules that all use +Absolute, Relative, doing the exact opposite is unnecessarily confusing for fairly little gain. I therefore think MST's deprecation plan should be adopted.

Since Mojo's role support is still young, now is a good time to deprecate the syntax before a large body of DarkPAN code starts to rely on this feature.

[kraih]

@latk Which deprecation plan?

[kraih]

For completeness sake, i'm with @marcusramberg on this and my vote is 👎.

[latk]

@kraih Any of MST's suggestions. Introducing a with_traits() method that takes +Absolute, Relative and deprecating with_roles() in its favour would be the simplest, least confusing approach in my opinion.

[mrenvoize]

I actually like that one of Mojo's aims is to leave the baggage behind and think afresh.

I'd rather have new functionality that's more intuitive to the first time user and have it blindingly obvious in the docs that it's the opposite/different from all prior art for the more seasoned developer.

[shadowcat-mst]

On Fri, Aug 18, 2017 at 12:33:08PM +0000, Martin Renvoize wrote:

I actually like that one of Mojo's aims is to leave the baggage behind and think afresh.

Right, but this isn't a question of 'think afresh' or 'intuitive', this is a question of 'sri accidentally implemented the feature backwards'.

I'd rather have new functionality that's more intuitive to the first time user and have it blindingly obvious in the docs that it's the opposite/different from all prior art for the more seasoned developer.

I still don't understand what's wrong with picking a different character.

Being in a situation where + means one thing in the rest of the codebase and the opposite in with_roles() is just actively cruel to first time maintainers of existing codebases.

OTOH I can see an argument for 'class names just mean class names'. Just then can we please use ~Foo or -Foo or .Foo or ... something other than

• so we avoid the collision?

-- Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue

http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/

Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN commercial support, training and consultancy packages could help your team.

[kraih]

Right, but this isn't a question of 'think afresh' or 'intuitive', this is a question of 'sri accidentally implemented the feature backwards'.

That doesn't mean the current implementation is not intuitive. I assumed it works this way because the opposite seemed silly.

[shadowcat-mst]

On Fri, Aug 18, 2017 at 06:46:02AM -0700, Sebastian Riedel wrote:

Right, but this isn't a question of 'think afresh' or 'intuitive', this is a question of 'sri accidentally implemented the feature backwards'.

That doesn't mean the current implementation is not intuitive. I just assumed it works this way because the opposite seemed silly.

Yes. I've never been exactly fond of it. Hence liking

$class->with_roles('Foo::Bar::Role');
$class->with_roles(qw(::Foo ::Bar));

since then it's 100% obvious that's a trailing-part-of-a-package-name.

-- Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue

http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/

Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN commercial support, training and consultancy packages could help your team.

[marcusramberg]

:: is pretty ugly to me :-/ On Fri, 18 Aug 2017 at 15:52, shadowcat-mst notifications@github.com wrote:

On Fri, Aug 18, 2017 at 06:46:02AM -0700, Sebastian Riedel wrote:

Right, but this isn't a question of 'think afresh' or 'intuitive', this is a question of 'sri accidentally implemented the feature backwards'.

That doesn't mean the current implementation is not intuitive. I just assumed it works this way because the opposite seemed silly.

Yes. I've never been exactly fond of it. Hence liking

$class->with_roles('Foo::Bar::Role'); $class->with_roles(qw(::Foo ::Bar));

since then it's 100% obvious that's a trailing-part-of-a-package-name.

-- Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue

http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/

Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN commercial support, training and consultancy packages could help your team.

—

You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kraih/mojo/issues/1122#issuecomment-323359608, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAVlrftN61pstMBIL46vt2GGtennnyKks5sZZcngaJpZM4O6gxv .

[Grinnz]

I prefer the bare class name to mean absolute as it currently does, but I am in favor of a different marker for relative, like :: or ~.

[jberger]

For the record, even though I think this is already voted down, I'm a :-1: on <symbol>Absolute. I think that is a confusing system and one I wish I hadn't done for WithRoles. I'm sure I did so for compatibility with other systems; since I don't use other systems I'm sure someone suggested to me that I do so.

That said, I'm not married to + and if it would be confusing I am not opposed to ~ or -. While I get the notion of ::, I don't really think I'm a fan. I can't quite place it, but it has something to do with :: meaning the root symbol table; I doubt that would confuse many people, but I see no reason to abuse symbols unnecessarily.

---

Personal opinion:

While I don't think Mojo needs to be beholden to or even greatly consider other projects, we have done well when we've actively tried to avoid confusion, even when that is painful. I think about the mass renaming in Mojo::DOM for example. Sure it was painful but it was the right move. I think that given that most of us agree that the convention of + to mean "use literally" is rather backwards to expectation; @kraih's accidental choice of the other convention is an example.

I think Mojo could lead the way here, adopting a new symbol and convention. If we chose a symbol that other projects could adopt (even just as an addition) we could help unify projects around a convention that makes sense. Therefore, after much consideration, in my perfect world I would have Mojo move to ~. This symbol make sense to me for two reasons. It is the concatenation operator in Perl 6 which isn't totally relevant but it is something. More interestingly to me it is part of the binding operator =~ in Perl 5 which denotes a sense of "matching" (and for better or worse, smartmatch operator too).

[jhthorsen]

I like "+", since I think of it in the context of extending something, such as has '+attr'. It also stands out visually to me that this isn't really the name of a package.

I also don't think that the plugin system, or other things like $dbic->resultset($moniker), is in the same usecase, since I consider them relative to the local system, while roles can come from "anywhere".

I want to keep it as is.

[kraih]

I think that settles it, we will keep with_roles the way it is now.