Dancer2 icon indicating copy to clipboard operation
Dancer2 copied to clipboard

Allow for serializers to live someplace other than Dancer2::Serializer.

Open perlpilot opened this issue 7 years ago • 3 comments

From IRC because I'm busy+lazy:

<perlpilot> If I write my own serializer, it must live in Dancer2::Serializer::Whatever?  That sucks.
<bigpresh> perlpilot: Well, the interface between it and D2 does.  Your serialiser itself could be any other dist, and D2::Serialiser::YourSerializer would just be a thin shim that interfaces with it
<bigpresh> Just like e.g. D2::Serialiser::JSON doesn't contain all the serialisation, it's just a shim between D2 and JSON (JSON::Any, IIRC)
<mst> bigpresh: being able to have a MyApp::Serializer::Whatever would be nice
<mst> this is another one of those "features for larger apps that Catalyst figured out ten years ago and Dancer seems to insist on learning the hard way" :(
<mst> STEAL OUR IDEAS ALREADY PLEASE
<skington> Is this the ability to say “MyApp::Serializer::Whatever is a serializer, so add it to the list, rather than only accepting serializers in the Dancer2::Serializer namespace”?
<mst> right
<skington> And you don’t want to have a small module called Dancer2::Serializer::Something in your code base because that looks weird?
<mst> no, because shitting over a global namespace that may result in a clash with cpan is (a) bad engineering (b) confusing (c) a footgun
<mst> it's one step less awful than monkey patching, but it's still pretty gross
<perlpilot> indeed.
<perlpilot> Also, in my case this app I'm working on has a strict separation of "stuff from CPAN" and "stuff invented for this app"   I can work around it, but it's just *another* thing that adds to the friction here.
<mst> right, precisely
<skington> Presumably you’d want to explicitly make people say e.g. set serializer => ‘MyApp::Serializer::Whatever’ in their config, so there was no potential confusion if someone also had Dancer2::Serializer::Whatever installed?
<mst> skington: the convention is to use '+' for full namespace
<mst> set serializer => '+MyApp::Serializer::Whatever'
<skington> That’s a confusing convention in this situation, given that set serializer => ‘Foo’ means “load serializer Dancer2::Serializer::Foo” but set serializer => ‘+Foo’ meanse “load serializer Foo”. We use addition precisely where we’re *not* adding any two things together.
<skington> I assume it made more sense in the other circumstances.
<SysPete> perlpilot: any chance of an issue so this doesn't get forgotten as I don't see why it should be so strict?
<mst> skington: I forget where it came from, but it's common to Catalyst, DBIx::Class, Moose etc.
<mst> skington: I dislike the convention, but being inconsistent with everything else would be worse
<mst> skington: I mean, ideally, you'd use '::Foo' to mean 'relative to a namespace search path' and 'Foo' would just mean 'Foo', which I've done, but that would require a TARDIS
<skington> I thought the + convention in Moose was for overriding an existing attribute? But yeah, once a convention is settled, there’s no point trying to mess with it.
<mst> hm, it's possible the Moose trait system just guesses, actually
<mst> but, yeah, I don't like '+' very much but it's better than being inconsistent or not having the feature at all
<skington> And you can’t just rely on there being a :: in the name somewhere to denote “this is a fully-qualified package name”.
<mst> and then I write Dancer2::Serializer::JSON::MaybeXS and then SATAN

perlpilot avatar Feb 17 '17 19:02 perlpilot

To this I would add that plugins currently require the namespace starting with Dancer2::Plugin::. I would be happy to resolve that too.

xsawyerx avatar Feb 18 '17 21:02 xsawyerx

@xsawyerx Plugins without a Dancer2::Plugin prefix was one the first changes applied after the release of the reworked plugin architecture (see #1175).

We should allow any engine to live in another namespace, not just serializers.

veryrusty avatar Feb 19 '17 06:02 veryrusty

Oh yeah, I forgot about that.

But I think there are still additional hard-coded entries for it:

The call for isa should be okay since it walks the tree, but using ref only returns most recent.

xsawyerx avatar Feb 19 '17 09:02 xsawyerx