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

Use of YAML.pm to read config is hard-coded (so YAML::XS is not used even when specified) #1163

Open
1nickt opened this issue Oct 11, 2016 · 2 comments

Comments

@1nickt
Copy link
Contributor

1nickt commented Oct 11, 2016

Hello,

<TL;DR>
The docs state you can specify YAML::XS for reading config, but Dancer::Config hard-codes use of YAML.
</TL;DR>

I have been trying to use YAML::XS instead of YAML for reading the Dancer config (mostly because YAML.pm doesn't support in-line comments as per the YAML spec).

The docs for Dancer::Config state:

By default, the module YAML will be used to parse the configuration files. If desired, it is possible to use YAML::XS instead by changing the YAML engine configuration in the application code:

config->{engines}{YAML}{module} = 'YAML::XS';

See Dancer::Serializer::YAML for more details.

The docs for Dancer::Serializer::YAMLstate:

By default, the module YAML will be used to serialize/deserialize data and the application configuration files. This can be changed via the configuration:

engines:
    YAML:
        module: YAML::XS

Note that if you want all configuration files to be read using YAML::XS, that configuration has to be set via application code:

config->{engines}{YAML}{module} = 'YAML::XS';

However, the code in Dancer::Config reads (starting around l.227) :

sub load_settings_from_yaml {
    my ($file) = @_;

    my $config = eval { YAML::LoadFile($file) }
        or confess "Unable to parse the configuration file: $file: $@";

... which seems to hard-code use of YAML.

Also, the code for Dancer::Serializer::YAML reads (starting around l. 43):

sub serialize {
    my ($self, $entity) = @_;
    YAML::Dump($entity);
}

sub deserialize {
    my ($self, $content) = @_;
    YAML::Load($content);
}

... which also seems to hard-code use of YAML.

I would be happy to submit a patch to fix this, if there is any interest from the maintainers in applying the fix.

@1nickt 1nickt changed the title Use of YAML.pm to read config is hard-coided (so YAML::XS is not used even when specified) Use of YAML.pm to read config is hard-coded (so YAML::XS is not used even when specified) Oct 11, 2016
@bigpresh
Copy link
Member

bigpresh commented Oct 11, 2016

Hmm. Given the feature-freeze in D1, I'm mostly inclined to say "we'll update the docs to remove the suggestion that you can override the use of YAML" - but I realise that doesn't help you with your requirement.

I'll give it a little thought - if it can be made possible to use YAML::XS by setting that setting before the config is read - but I have a vague recollection that calling Dancer->config, even when using it as a mutator?

As mentioned on IRC, looks like PR #1072 already implemented it, but was closed by f0585d5 - so at least most of the work is already done it would seem.

@1nickt
Copy link
Contributor Author

1nickt commented Oct 12, 2016

Hi @bigpresh, PR raised here :-)

I didn't add support for YAML::XS in session management, just reading config and serialization. So now I can have in-line comments in my YAML, whee!

All tests passing and live app round-tripping YAML content also tested.

If it's not pushing it and you could see your way clear to having a look at this earlier PR as well, that'd be brilliant.

Thanks!

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

No branches or pull requests

2 participants