perlpunk / YAML-PP-p5

A YAML 1.2 processor in perl
https://metacpan.org/pod/YAML::PP
24 stars 8 forks source link

Implement loading of schema classes not prefixed with YAML::PP::Schema #8

Closed pplu closed 5 years ago

pplu commented 5 years ago

Hi,

As commented in https://github.com/pplu/cfn-perl/issues/23, I've had a shot at loading Schemas in custom namespaces. I've diverged from you initial proposal because it would have changed all the current loading of standard schemas (having to prepend them with ':'. I've used the '+' sign to signal that the class should be loaded without prepending YAML::PP::Schema:: to it. Using the + sign remembered me of other modules (Catalyst?, that uses similar schemes for specifying custom namespaces for plugins).

Hope this is helpful :)

perlpunk commented 5 years ago

Ok, I need to think more about that one. I don't want to use the + sign, because I already have reserved this for another purpose. I want people to allow enabling/disabling certain tags/features, so for example:

YAML::PP->new ( schema => [qw/ Core +!!timestamp -!!float /] )
YAML::PP->new ( schema => [qw/ JSON Perl +loadcode /] )

Does that make sense? So I think the : colon would be better. I think Dist::Zilla uses this also.

pplu commented 5 years ago

Changed + for :. There is one doubt I have about the design of this feature: The external Schema modules already have to be loaded via use/require, so the usage is as follows:

use YAML::PP;
use MySchema;

my $yp = YAML::PP->new(
    schema => [qw/ :MySchema /],
);

I have no problem with this as long as it's clear in the documentation, which I'll write once we get this feature sufficiently drafted out. I'd like to know your considerations around who's responsibility should loading the Schema module be. The caller? or YAML::PP via https://metacpan.org/pod/Module::Runtime, for example?

perlpunk commented 5 years ago

I'd like to know your considerations around who's responsibility should loading the Schema module be.

Yes, my plan is to use Module::Load (because it's in core), so users don't have to load it themselves.

pplu commented 5 years ago

Yes, my plan is to use Module::Load (because it's in core), so users don't have to load it themselves.

I've pushed a version that uses Module::Load. A couple of observations:

perlpunk commented 5 years ago

Sorry that this is going so slow, I've been busy...

Should we split them into their own files, and let all schemas be dynamically loaded?

I was thinking about having a hash of already loaded schemas, and then load all other schemas. At some point, they should be split to their own files, I think. I just wasn't sure if I'll go with the class hierarchy, and still not sure, which schema should be the default (Core or JSON).

perlpunk commented 5 years ago

Thanks, merged!