Skip to content
This repository has been archived by the owner on Aug 24, 2021. It is now read-only.

feat: use RFC3339 to format dates, fixes ipfs/go-ipld-git#16 #43

Merged
merged 1 commit into from
Mar 27, 2019

Conversation

sameer
Copy link
Contributor

@sameer sameer commented Mar 19, 2019

Related to ipfs/go-ipld-git#32. Use RFC3339 to format dates in JSON output.

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #43 into master will increase coverage by 0.19%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
+ Coverage   92.03%   92.23%   +0.19%     
==========================================
  Files           7        7              
  Lines         314      322       +8     
==========================================
+ Hits          289      297       +8     
  Misses         25       25
Impacted Files Coverage Δ
src/util/util.js 93.47% <100%> (+1.37%) ⬆️

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 4519644...87081b8. Read the comment docs.

@vmx
Copy link
Member

vmx commented Mar 20, 2019

Is there an alternative to the moments module? It adds quite a lot to the bundle size (approx. 300K).

Do I understand the issue with tests could be fixed with updating the fixtures?

@sameer
Copy link
Contributor Author

sameer commented Mar 20, 2019

Is there an alternative to the moments module? It adds quite a lot to the bundle size (approx. 300K).

I will look for an alternative method.

Do I understand the issue with tests could be fixed with updating the fixtures?

Looked into this -- turns out I confused the purpose of serialize & deserialize; I thought serialize produced the dagNode, when that's what deserialize does so the logic is reversed. I have pushed and fixed this, so the fixtures don't need to be updated.

@sameer
Copy link
Contributor Author

sameer commented Mar 20, 2019

Ok, took me a few hours but I wrote functions for converting between raw timestamps and ISO8601 ones. I'm not convinced that it's completely correct so probably will need some more tests to verify it.

@rvagg
Copy link
Member

rvagg commented Mar 20, 2019

https://github.com/samsonjs/strftime is usually the go-to when you only need to format time strings, it might be safer than rolling our own date formatter

@sameer
Copy link
Contributor Author

sameer commented Mar 20, 2019

https://github.com/samsonjs/strftime is usually the go-to when you only need to format time strings, it might be safer than rolling our own date formatter

Good point, I'll update the PR with that in a minute.

@sameer
Copy link
Contributor Author

sameer commented Mar 21, 2019

Ok, that made things a lot simpler. Thanks @rvagg!

src/util/util.js Outdated Show resolved Hide resolved
@sameer sameer changed the title Use RFC3339 to format date in serializePersonLine, fixes ipfs/go-ipld-git#16 (WIP) feat: Use RFC3339 to format date in serializePersonLine, fixes ipfs/go-ipld-git#16 Mar 21, 2019
@sameer sameer changed the title feat: Use RFC3339 to format date in serializePersonLine, fixes ipfs/go-ipld-git#16 feat: use RFC3339 to format dates, fixes ipfs/go-ipld-git#16 Mar 21, 2019
@vmx
Copy link
Member

vmx commented Mar 25, 2019

From the size increase perspective (about 16KB) it looks fine to me. It would be great if you @magik6k could check if it is correct (I don't really have a clue what's going on :)

@vmx vmx merged commit 8a9f7cb into ipld:master Mar 27, 2019
@vmx
Copy link
Member

vmx commented Mar 27, 2019

Thanks a lot @sameer for this change! I added a "BREAKING CHANGE" to the commit message, so that it shows up in the changelog.

@sameer sameer deleted the dateformat branch March 27, 2019 14:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants