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

Use RFC3339 to format dates, fixes #16 #32

Merged
merged 1 commit into from
Sep 27, 2019
Merged

Conversation

sameer
Copy link
Contributor

@sameer sameer commented Dec 30, 2018

Fixes #16 by using a Unix timestamp with a parsed offset from UTC. I added two tests for it to match up with git log --date=iso-strict and git log --date=raw.

Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So. while this technically is a breaking change, I don't think it will break anything too severely since I don't think there are many places that parse this from json.

This should also apply too https://github.com/ipfs/go-ipld-git/pull/32/files#diff-d280db36f672412d557878c23067458dL80

Also, do you mind looknig at how to port this to https://github.com/ipld/js-ipld-git? Thanks.

@sameer
Copy link
Contributor Author

sameer commented Jan 6, 2019

So. while this technically is a breaking change, I don't think it will break anything too severely since I don't think there are many places that parse this from json.

Other than someone looking at it manually, where else would this be relevant?

This should also apply too https://github.com/ipfs/go-ipld-git/pull/32/files#diff-d280db36f672412d557878c23067458dL80

Ok, I'll fix that.

Also, do you mind looking at how to port this to https://github.com/ipld/js-ipld-git? Thanks.

Sure, I can look into it.

@sameer sameer force-pushed the dateformat branch 3 times, most recently from 191186c to fa91985 Compare January 6, 2019 19:12
@sameer
Copy link
Contributor Author

sameer commented Mar 13, 2019

I looked into doing this for js-ipld-git, there are two problems:

  • Dates in JS are represented only in the local timezone, so it's hard to convert from raw timestamps to RFC3339. I would like to add momentjs which converts to RFC3339 with a one-liner (js-ipld-git/src/util/util.js line 43 changes from parts.push(node.date) to parts.push(moment.parseZone(node.date, "X ZZ").format()))

  • js-ipld-git/test/parse.spec.js line 98 can never pass. If I serialize to RFC3339, the cid from the original hash of an object, which was computed with the raw timestamp, will always be different than the one computed using RFC3339 timestamps.

@magik6k
Copy link
Member

magik6k commented Mar 19, 2019

I don't think js team will be against adding the moment library, it's small and MIT licensed so it should be fine.

Not preserving timezone information in parsed objects is a bit of a problem, would https://momentjs.com/timezone/ work for that?

@sameer
Copy link
Contributor Author

sameer commented Mar 19, 2019

I don't think js team will be against adding the moment library, it's small and MIT licensed so it should be fine.

Ok, sounds good

Not preserving timezone information in parsed objects is a bit of a problem, would https://momentjs.com/timezone/ work for that?

The timezone information thankfully is preserved as is, all the tests pass except the one I talked about which would have to be disabled. It's because the sha hash (filename of the test object file in this case I think) was found using a raw timestamp but then when the test recomputes it with the RFC3339 timestamp in the serialized output, it is incorrect.

Edit: I opened my changes as a PR ipld/js-ipld-git#43

@sameer sameer changed the title Use RFC3339 to format date in PersonInfo JSON, fixes #16 Use RFC3339 to format dates, fixes #16 Mar 21, 2019
commit.go Show resolved Hide resolved
@sameer
Copy link
Contributor Author

sameer commented Sep 27, 2019

@magik6k Sorry to bug about this again, the js version is merged so wondering if this can be merged too

@Stebalien Stebalien merged commit ccba3f1 into ipfs:master Sep 27, 2019
@sameer sameer deleted the dateformat branch September 27, 2019 04:43
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.

Date format
3 participants