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.

Sign up for GitHub

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

Jump to bottom

Add native implementation of format #83

Closed
wants to merge 1 commit into from
Closed

Add native implementation of format #83

wants to merge 1 commit into from

Conversation

Amorymeltzer
Copy link
Contributor

Closes #60

@coveralls
Copy link

Pull Request Test Coverage Report for Build 204

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 203: 0.0%
Covered Lines: 43
Relevant Lines: 43
💛 - Coveralls

@coveralls
Copy link

coveralls commented Oct 28, 2019 •
edited

Pull Request Test Coverage Report for Build 217

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 216: 0.0%
Covered Lines: 43
Relevant Lines: 43
💛 - Coveralls

@Amorymeltzer
Copy link
Contributor Author

Amorymeltzer commented Oct 28, 2019 •
edited

I think failures on node 8 are related to not having full intl support, see: nodejs/node#8500 and nodejs/node#19214.

Not sure if specifying en-US would fix that.

cylim
cylim approved these changes Oct 30, 2019
View reviewed changes
Copy link
Member

@cylim cylim left a comment

Choose a reason for hiding this comment

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

@stevemao should we remove Nodejs 8.0, the LTS end-of-life is December 2019. Two months from now.

@stevemao
Copy link
Member

@cylim Yes good idea :)

ODudek
ODudek reviewed Oct 30, 2019
View reviewed changes
__tests__/index.js
);
expect(m).toBe(n);
Copy link
Member

Choose a reason for hiding this comment

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

This test failed. Native new Date does not support am/pm as far as i know :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passed/passes for me (and in Travis?), but does adding hour12: true in the object do it for you?

@cht8687
Copy link
Member

cht8687 commented Feb 25, 2020

could you resolve the conflicts? so that we can merge

@Amorymeltzer
Copy link
Contributor Author

@cht8687 Done; apologies for not doing so earlier, I had thought this stalled waiting for removal of node 8

@Amorymeltzer
Copy link
Contributor Author

Conflict comes from #116, which largely (though not equivalently?) supersedes this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevemao stevemao stevemao approved these changes

@andrew-yangy andrew-yangy Awaiting requested review from andrew-yangy

@cht8687 cht8687 Awaiting requested review from cht8687

@ODudek ODudek Awaiting requested review from ODudek

@cylim cylim Awaiting requested review from cylim

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Native Date format using toLocaleDateString
6 participants
@Amorymeltzer @coveralls @stevemao @cht8687 @cylim @ODudek