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

date updates #79

Merged
merged 9 commits into from
Nov 19, 2020
Merged

date updates #79

merged 9 commits into from
Nov 19, 2020

Conversation

hardy925
Copy link
Contributor

Fixes

Please link to the issue this PR addresses

Description

better ISO 8106 support

Proposed changes in this PR

Things to look at

  • Test coverage
  • Code Style
  • Documentation (READMEs etc)

@hardy925 hardy925 added bug Something isn't working enhancement New feature or request v1.0.0 roadmap labels Nov 18, 2020
@hardy925 hardy925 self-assigned this Nov 18, 2020
Copy link
Collaborator

@ChrisDufourMB ChrisDufourMB left a comment

Choose a reason for hiding this comment

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

This doesn't appear to support all the different kinds of timezones that could be part of an ISO date, what degree of support are you looking to implement? I feel like we will need more tests cases for all the different variants and edge cases that can happen

Here is the test I tried, for reference

test({
  name: "iso8601Date parses with added timezone - 2020-12-31T12:00:00Z-06:00",
  fn() {
    const testJson = `{"date":"2020-12-31T06:00:00Z-06:00"}`;
    const testObj = JSON.parse(testJson, (_, v) => iso8601Date(v));
    assert(testObj.date instanceof Date);
    assertEquals(testObj.date.toISOString(), "2020-12-31T12:00:00.000Z-06:00");
  },
});

from_json/date.ts Outdated Show resolved Hide resolved
from_json/date.ts Outdated Show resolved Hide resolved
from_json/date.ts Outdated Show resolved Hide resolved
from_json/date.ts Outdated Show resolved Hide resolved
from_json/date.ts Outdated Show resolved Hide resolved
from_json/date.ts Outdated Show resolved Hide resolved
from_json/date_test.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@ms1111 ms1111 left a comment

Choose a reason for hiding this comment

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

Would you also want to have a serializer in the library to render a local date as an ISO date string (YYYY-MM-DD) in toJson()? Or would you want apps to provide that themselves (like our localDateToIsoDateString() function)

Here's the test case for localDateToIsoDateString() btw:

    // 1990-12-11
    const localDateMidnight = new Date(1990, 11, 11);

expect(localDateToIsoDateString(localDateMidnight)).toEqual("1990-12-11");

code:

export function localDateToIsoDateString(date: Date): string {
    const year = date.getFullYear();
    const month = date.getMonth() + 1;
    const day = date.getDate();
    return `${formatISOYear(year)}-${padLeft(month, 2)}-${padLeft(day, 2)}`;
}

from_json/date.ts Outdated Show resolved Hide resolved
from_json/date_test.ts Show resolved Hide resolved
@hardy925
Copy link
Contributor Author

@ms1111 I think we'll go the route of having authors write their strategies, the two we provide for dates is more than enough IMHO.

@hardy925 hardy925 merged commit e7186e8 into develop Nov 19, 2020
@hardy925 hardy925 deleted the date_update branch November 19, 2020 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants