Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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!

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

No branches or pull requests

2 participants