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.

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

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

Open
perlpilot opened this issue Feb 17, 2017 · 3 comments
Open

Comments

@perlpilot
Copy link
Contributor

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
@xsawyerx
Copy link
Member

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

@veryrusty
Copy link
Member

@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.

@xsawyerx
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants