Use RFC3339 to format dates, fixes #16

ipfs / go-ipld-git

ipld handlers for git objects

MIT License

57 stars 19 forks source link

Use RFC3339 to format dates, fixes #16 #32

Closed sameer closed 5 years ago

sameer commented 5 years ago

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.

sameer commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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