dump more configs in ::Role::MetaProvider::Provider

[karenetheridge]

I was going to send you a PR but the code is a spiderweb that I cannot navigate.

• dump_config should use numbers rather than strings when dumping boolean options
• the role's :version should be included, because it comes from a different distribution than the consuming plugin and the specific version might be very significant for feature compatibility and debugging incompatibilities.

[karenetheridge]

From kentnl/Dist-Zilla-Plugin-MetaProvides-Package#6 (comment):

Wouldn't traversing the stack of roles applied to plugins and extracting their versions be a job for https://metacpan.org/source/RJBS/Dist-Zilla-5.039/lib/Dist/Zilla/Plugin/MetaConfig.pm#L42-50

It doesn't know how to do that. dump_config is just a method, and plugins and roles wrap that method to add their extra data. I am suggesting that the extra roles that the plugin brings in should have:

around dump_config => sub {
    my ($orig, $self) = @_;
    my $config = $self->$orig;
    $config->{+__PACKAGE__} = {
        # ... other configs that this role defines
        blessed($self) ne __PACKAGE__ ? ( version => $VERSION ) : (),
    };
};

[kentfredric]

It doesn't know how to do that. dump_config is just a method, and plugins and roles wrap that method to add their extra data. I am suggesting that the extra roles that the plugin brings in should have

Correct. However, MetaConfig already extracts various versions of packages, I'm just suggesting that if people want versions from composed roles, that it might be a job for MetaConfig, not every single role ever.

[kentfredric]

Also, dump_config returning strings instead of numbers is not something I feel is entirely my responsibility:

• The Type constraint they're stored in asserts they must be boolean values.
• The values of all attributes are just dumped as-is without casting to numbers or strings.

Which means that being numbers or strings is really a side effect of how they're declared.

I don't know, I can't think today.

[karenetheridge]

Correct. However, MetaConfig already extracts various versions of packages, I'm just suggesting that if people want versions from composed roles, that it might be a job for MetaConfig, not every single role ever.

Perhaps, but it's not doing it today (it doesn't dump versions for anything but the plugin it is calling, and knows nothing about any roles used to compose it or if they modify dump_config thelselves), and I doubt rjbs would really go for something that complicated. For now at least, we need to add all data to the hash ourselves that we think is relevant.

Also, dump_config returning strings instead of numbers is not something I feel is entirely my responsibility:

It just looks nicer in META.json to see { "foo" : 0 } rather than { "foo" : "0" } for something that is a bool. That's all.

[kentfredric]

I think another objection I have is that this would be adding a meta-key in each packages metadata that doesn't map anywhere to an attribute, and that could be confusing.

[karenetheridge]

This is what I do:

https://metacpan.org/source/ETHER/Dist-Zilla-Role-FileWatcher-0.006/lib/Dist/Zilla/Role/FileWatcher.pm#L53-65
https://metacpan.org/source/ETHER/Dist-Zilla-PluginBundle-Author-ETHER-0.105/META.json#L1213-1222

[kentfredric]

Another reason I don't want it to be so terse, is because my configuration extraction is centralised, and so, injecting version generically is not always a good idea, because version may already exist with another purpose.

For instance: https://metacpan.org/source/KENTNL/Dist-Zilla-Plugin-MetaYAML-Minimal-0.001000/lib/Dist/Zilla/Plugin/MetaYAML/Minimal.pm#L25

^ Already has version with a specific meaning. Having config_dumper inject version on top of this in any condition would be bad.

So a better name must be chosen to eliminate conflict risk.

[karenetheridge]

I don't understand the issue with [MetaYAML::Minimal] -- there is no role being consumed there that would want to dump its version into configs.

I would suggest your configdumper sub should simply allow extra keys and values to be added to whatever data it dumps, and version added to that list where it's relevant (just in roles that live in other distributions, like here).

[kentfredric]

Its not when MetaYAML::Minimal consumes things that it becomes necessary.

Its when it is consumed that it becomes necessary.

For instance, if I subclassed it, and had the config_dumper go "Ok, you've been subclassed, you should show what version is being used and export it" , and its then the explosion would happen.

[karenetheridge]

I'm not interested in arguing with you why your convoluted architecture is not capable of accomodating the problem. My request was just that this particular role document its version, to aid in debugging in case a version incompatibility was the source of the problem. It's clear now that this wasn't relevant in my problem today, and you don't want to do it.

[kentfredric]

and you don't want to do it.

That's not the case. I do. But I want to do it everywhere that it makes sense to, not just one place.

Nothing about architecture is "incapable", I can do it on a per-role basis easily. 0fe34a7#diff-d7a31d8ac610001790ab8b6c84658821R254

But I want to do it in more than just that one place, because its a useful thing to do.