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

Add timezone to formatISO #1576

Merged
merged 9 commits into from
Jan 8, 2020

Conversation

imballinst
Copy link
Contributor

Fixes #1559.

Signed-off-by: Try Ajitiono [email protected]

Signed-off-by: Try Ajitiono <[email protected]>
@imballinst imballinst self-assigned this Dec 22, 2019
Signed-off-by: Try Ajitiono <[email protected]>
src/formatISO/index.js Outdated Show resolved Hide resolved
src/formatISO/test.js Outdated Show resolved Hide resolved
@imballinst
Copy link
Contributor Author

I'll fix the CI test slightly later™

Signed-off-by: Try Ajitiono <[email protected]>
Signed-off-by: Try Ajitiono <[email protected]>
Signed-off-by: Try Ajitiono <[email protected]>
Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

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

Please update JSDoc as well.

src/formatISO/index.js Outdated Show resolved Hide resolved
src/formatISO/test.js Outdated Show resolved Hide resolved
@@ -12,7 +12,7 @@ import addLeadingZeros from '../_lib/addLeadingZeros/index.js'
*
* @param {Date|Number} date - the original date
* @param {Object} [options] - an object with options.
* @param {'extended'|'basic'} [options.format='extended'] - if 'basic', hide delimiters between date and time values.
* @param {'extended'|'basic'} [options.format='extended'] - if 'basic', hide delimiters between date and time values and hide time zone.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kossnocorp I have updated the JSDoc comments. Please take a look!

Copy link
Member

Choose a reason for hiding this comment

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

Oh god damn, that's the line that confused me. The docs are just misleading. I'll fix that in the master.

@imballinst imballinst requested a review from kossnocorp January 5, 2020 07:17
@kossnocorp
Copy link
Member

@imballinst please hold on, the review is coming. I don't like the fact that timezone presence is tied to delimiters, because they aren't connected. But I can't find a solution yet, and without a solution, the review is pointless.

I hope @leshakoss can help me here because it was he who developed the current argument schema.

@imballinst
Copy link
Contributor Author

@imballinst please hold on, the review is coming. I don't like the fact that timezone presence is tied to delimiters, because they aren't connected. But I can't find a solution yet, and without a solution, the review is pointless.

I hope @leshakoss can help me here because it was he who developed the current argument schema.

Sorry for that! I thought you missed this. Thanks for clarifying!

Could you tell more about "timezone tied to delimiters"? Isn't the timezone only tied to the time and complete representation (regardless of basic or extended format)?

Alternatively, we could add something like, includeTimezone field in the options object. But yeah, I agree, this needs to be discussed. Perhaps there is a best way to do that.

@kossnocorp
Copy link
Member

Could you tell more about "timezone tied to delimiters"? Isn't the timezone only tied to the time and complete representation (regardless of basic or extended format)?

You're right I completely missed it. I'm not even sure what I was talking about lol. Sorry about that.

I took a second look at the code and the current behavior is what we need.

Sorry again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

formatISO omitting final Z character in dates
3 participants