jhthorsen / json-validator

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

Fix for deep recursion on Windows #170 #180

Closed charsbar closed 4 years ago

charsbar commented 4 years ago

Summary

This is the modified version of #178 by @chorny.

Motivation

The point of this fix is to use the same type of id as the _load_schema() method returns at the line 342 on a case tolerant file system.

I wanted to add a test that uses MixedCase files but simple mocking of CASE_TOLERANT constant doesn't work under a case intolerant system because _load_schema fails to find the other schema file because of this case lowering. Removing case lowering of the line 342 would also work, but considering of #102, I assume it's better to keep it as is. Maybe we can consider t/more-bundle.t as a required failing test. Some of the Win32 reports say the tests have "pass"ed but that's just because YAML::XS is not installed there and the test is skipped.

References

170, #178, #102

jhthorsen commented 4 years ago

Can’t you just do something like this in the test?

plan skip_all => 'not case tolerant' unless JSON::Validator::CASE_TOLERANT;

charsbar commented 4 years ago

Skipping a test doen't fix anything actually, and people can install this module without testing already (if they really need this, like me).

I'm not sure why you suggest skipping, but if you need to be assured before merging, GitHub Actions or AppVeyor may help.

jhthorsen commented 4 years ago

CPAN testers will still run the test, and let me know in case I remove that logic in the future. And the test will be run for anyone who it’s relevant for.

charsbar commented 4 years ago

You've added the CASE_TOLERANT trick for Windows (#102), instead of skipping t/bundle.t. And then t/more-bundle.t added recently revealed the previous fix was insufficient. I believe it's more appropriate to fix it again.

charsbar commented 4 years ago

Sorry, I pushed the wrong button.

jhthorsen commented 4 years ago

Oh! I haven’t noticed that CPAN testers have failing tests on windows.

charsbar commented 4 years ago

Thank you!