search

jhthorsen / json-validator

:cop: Validate data against a JSON schema
https://metacpan.org/release/JSON-Validator
56 stars 58 forks source link

Added missing prereqs as reported by CPANTS. #88

Closed manwar closed 6 years ago

manwar commented 6 years ago

Hi @jhthorsen

Please review the PR.

Many Thanks. Best Regards, Mohammad S Anwar

jhthorsen commented 6 years ago

I can't see how the modules you point out is really missing. Can you elaborate?

manwar commented 6 years ago

Hi @jhthorsen

Here is link to CPANTS that says there are missing prereqs.

https://cpants.cpanauthors.org/release/JHTHORSEN/JSON-Validator-1.08

Best Regards, Mohammad S Anwar

jhthorsen commented 6 years ago

Though it looks and sounds like quality, higher Kwalitee score doesn't always mean a distribution is more useful for you.

So I don't see how adding those deps increases the quality of the package. Do you actually have issues during installation?

manwar commented 6 years ago

I agree with your view on Kwalitee.

However I have come across many distributions that do explicitly list all prereq even if they are in CORE e.g strict, warnings.

Therefore, I thought, it would look nice and clean, when it has all prereqs listed explicitly. There is no harm adding them even if its part of CORE, in my humble opinion.

I am not using the distribution, just saw on metacpan.org and came across Kwalitee listing missing prereqs.

jhthorsen commented 6 years ago

If that is your view, then this PR is incomplete (Missing File::Find) -- I however do not share this view, unless a given version is required.

Sorry.

manwar commented 6 years ago

Points well taken and sorry for inconvenience,

jhthorsen commented 6 years ago

Not at all: I do appreciate you taking the time and contributing!

Thanks for the interest 👍

perlpunk commented 6 years ago

Just a comment: The prereqs list does not only serve the purpose of listing the modules that need to be installed, but they are also a documentation of what modules are used, without being forced to search the code. This can be used for statistics.

Also, theoretically, modules can be removed from core later; and some distributions (for example fedora/redhat/centos) don't include some core modules in the perl package (although I think this was fixed in recent versions).

Of course, modules that are optional, should not be listed as a prereq (but maybe as "recommends" or "suggested").

Edit: Since the JSON::Validator::OpenAPI::Dancer2 module is part of this distribution, Hash::MultiValue is actually a requirement, formally.

jhthorsen commented 6 years ago

Can't beat two against one ;)

I'll take a PR if it requires overload, strict, File::Find, File::Spec and File::Temp with the minimum versions. I think I would just find the versions in perl 5.10 and use those.

If you want to do more, you can rewrite the cases where File::Find, File::Spec and File::Temp is used and replace it with Mojo::File.

Also, Hash::MultiValue can be listed as suggested if you like.

See also https://metacpan.org/pod/distribution/Module-CPANfile/lib/cpanfile.pod#requires,-recommends,-suggests,-conflicts

@perlpunk: I disagree: Hash::MultiValue is only required in a special usecase of this distro, and in that case, it will already be installed.

perlpunk commented 6 years ago

@jhthorsen you're right, that's why I said "formally" =)

btw, finding out all prereqs can be done with:

scan-perl-prereqs-nqlite . --json
scan-perl-prereqs-nqlite . --cpanfile