Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dates (without times) and times (without dates) #10

Closed
wants to merge 3 commits into from

Conversation

novas0x2a
Copy link

I found two separate but related problems, the first is a date without a time (fixed by adding datetime.date to the JSONDateTimeEncoder; the second a time without a date, which yaml refers to as a sexagesimal. YAML 1.1 specifies that these parse as base 60 integers, and can't easily be round-tripped. I fixed that by switching to an api-compatible yaml loader that supports yaml 1.2, which eliminates sexagesimals among other things.

@codecov-io
Copy link

codecov-io commented Dec 13, 2017

Codecov Report

Merging #10 into master will not change coverage.
The diff coverage is 75%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #10   +/-   ##
=======================================
  Coverage   84.72%   84.72%           
=======================================
  Files           2        2           
  Lines          72       72           
=======================================
  Hits           61       61           
  Misses         11       11
Impacted Files Coverage Δ
yq/__init__.py 84.5% <75%> (ø) ⬆️

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 7c2df8f...c98e32a. Read the comment docs.

@kislyuk
Copy link
Owner

kislyuk commented Dec 13, 2017

Thank you for reporting. I'm not ready to switch to ruamel.yaml though. We will need a different solution for now.

@kislyuk kislyuk closed this in c01a4b3 Dec 27, 2017
@kislyuk
Copy link
Owner

kislyuk commented Dec 27, 2017

Thank you again for reporting and working on this, @novas0x2a. I finally got around to fixing the issue with bare dates. As I mentioned though, I'm not ready to switch to ruamel.yaml yet - I'd like to wait a few more months for https://github.com/yaml/pyyaml to show signs of life. Until then, I've added a test case marked as an expected failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants