Improve date parsing

[helrond]

Attempts to use fromisoformat to parse date strings. The (buggy) strptime method is used as a fallback for Python versions < 3.7

fixes #685

[codecov]

Codecov Report

Merging #722 into master will decrease coverage by 0.17%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master     #722      +/-   ##
==========================================
- Coverage   96.18%   96.01%   -0.18%     
==========================================
  Files          17       17              
  Lines        2648     2658      +10     
  Branches      308      310       +2     
==========================================
+ Hits         2547     2552       +5     
- Misses         84       87       +3     
- Partials       17       19       +2     

Impacted Files	Coverage Δ
jsonschema/tests/test_jsonschema_test_suite.py	93.02% <50.00%> (-6.98%)	⬇️
jsonschema/_format.py	77.27% <60.00%> (-0.57%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff1ddcf...d9d5fe1. Read the comment docs.

[Julian]

@helrond now that these are merged upstream think you can now unskip the relevant tests and help get this over the finish line?

Appreciated!

[helrond]

@Julian sure - what exactly do I need to do?

[Julian]

@helrond so I basically pulled in the changes you made to the test suite, so now those tests are in here, but being skipped on master (where they'd otherwise fail).

Essentially what needs to be done now is to unskip them (delete e.g. these lines: https://github.com/Julian/jsonschema/blob/master/jsonschema/tests/test_jsonschema_test_suite.py#L103-L111), confirm that the tests now pass in this branch, and then we'd be good.

Lemme know if that makes sense!

[helrond]

OK, I think I did that right, but tests are failing on everything < Python 3.6, which I believe is a consequence of the implementation we chose, which falls back to the buggy strptime method if fromisoformat isn't available.

[Julian]

Got it so... probably the thing to do is to re-add a skip that just skips those tests on 3.6 then? Have a look at e.g. the narrow_unicode skipping thing, it should show you how to do that (though yours will now predicate on sys.version_info)

[helrond]

OK, so tests are passing now but I'm getting an error on test coverage. Any pointers on how to fix that?

[Julian]

Cool! We can likely ignore that, it's due to the fact that we don't yet combine coverage across all runs, so codecov is confused and thinks coverage has gone down, it won't block merging though.

Let me review! Will leave any comments.

[Julian]

Minor, but might as well -- it'll save doing the "wrong" thing on each call on Py36 if we do this only once rather than each time we validate.

I.e. if we move this into a try/except, and do:

if hasattr(datetime.date, "fromisoformat"):
     _is_date = datetime.date.fromisoformat
else:
    def _is_date(instance):
        return datetime.datetime.strptime(instance, "%Y-%m-%d")

def is_date(...):
    return _is_date(...)

[helrond]

I'm not sure I fully got what you were after here, but happy to take another crack at it if I messed up.

[Julian]

Sorry, I should have been less lazy in writing it out :) -- so the point is (which is true of your commit here too) -- as is, every time the is_date validation is applied, it checks to see which implementation should be used. So on Py36, that means you first try fromisoformat, then that fails, then you try the old way, but then you "forget" about the fact that it didn't work and the next time the function is called, you'll again try the fromisoformat, again find it doesn't work, etc.

Instead, what I was trying to say was to move the check outside of the is_date function entirely. In other words, globally at the top level in the module:

if hasattr(datetime.date, "fromisoformat"):
     _is_date = datetime.date.fromisoformat
else:
    def _is_date(instance):
        return datetime.datetime.strptime(instance, "%Y-%m-%d")

@_checks_drafts(draft3="date", draft7="date", raises=ValueError)
def is_date(instance):
    if not isinstance(instance, str):
        return True
    return _is_date(instance)

Now, we only check once on import which implementation should be used, and then if you call that function 1000 times, we've already figured out the right implementation ahead of time.

Does that make more sense?

[helrond]

Ah yes, got it. If I'd looked around a bit more in that file I'd have seen you doing that kind of thing in other spots. This one should be resolved now.

[Julian]

Two minor nitpicks on the implementation that I should have pointed out earlier.

After that though think we'll be good to merge.

Much appreciated again...

[Julian]

All good, this looks great now, thanks again!