jhthorsen / json-validator

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

pod fix, use sharedirs, better schema inference #150

Closed karenetheridge closed 5 years ago

karenetheridge commented 5 years ago

I am quite happy to split these up into multiple pull requests for a nicer git history.

jhthorsen commented 5 years ago

Hi, thanks for helping out, but I’m not sure I’m a fan of this PR. I’ll look into it closer, but my main question is: Why do I want the cache files in a share/ dir? Which problem does that solve?

Also, when you make a PR, then you don’t have to update Changes and Makefile.PL. Just add the new dependencies to the cpanfile instead, since it makes it easier to merge. (Note however that you don’t have to undo those changes now. Let’s rather discuss the content of this PR first)

karenetheridge commented 5 years ago

As described in the commit message, data files do not belong in the normal heirarchy next to .pm files. Some tools are confused by this, and we (the Perl Toolchain Gang) have been trying to get distributions to switch to the standard File::ShareDir as much as possible.

karenetheridge commented 5 years ago

There is no cpanfile in this repository -- the only place prerequisites are indicated is in Makefile.PL.

jhthorsen commented 5 years ago

Ah. Right. I forgot that the cpanfile was removed when I move the repo over to mojolicious.

What is “Some tools”? You’re not really selling File::ShareDir. Not sure if this is fixed, but I’ve previously avoided that module, since it could never figure out that I was doing development and would constantly fetch the installed files, instead of the repo files.

jhthorsen commented 5 years ago

File::Share seems to fix this issue, but I’m surprised that there has to be so much code and so many non-core dependencies to do something that seems quite simple.

karenetheridge commented 5 years ago

since it could never figure out that I was doing development and would constantly fetch the installed files, instead of the repo files

That's where Test::File::ShareDir comes in -- it works some magic underneath to use the local files in testing, rather than using whatever the last version had installed (if anything).

Sawyer wrote a post on FSD a few years ago here -- http://blogs.perl.org/users/sawyer_x/2011/01/starting-to-use-filesharedir.html It's also referenced in https://metacpan.org/pod/ExtUtils::MakeMaker#SEE-ALSO as the preferred method for installing data files that go with a distribution install. It's considered part of the Perl toolchain and is well-maintained.

I see it's still dragging in Class::Inspector though, just for an old crufty deprecated feature -- I'll look into getting that fixed up right away!

karenetheridge commented 5 years ago

FWIW, the bits of this PR that I really care about is adding the automatic handling of "$schema" in the document. The other things are just things I noticed while I was in the code and are far less important to me. :)

jhthorsen commented 5 years ago

Please do open a new PR with what you really want changed/fixed, since it will be faster/easier to accept/reject. You can leave this unchanged, or force push without that change.

karenetheridge commented 5 years ago

I will open new PRs to replace these.