Skip to content

Update TimeSpan implementation to be consistent with number#519

Merged
sebastienros merged 10 commits intosebastienros:mainfrom
danielmpetrov:bugfix/gh-517
Nov 30, 2022
Merged

Update TimeSpan implementation to be consistent with number#519
sebastienros merged 10 commits intosebastienros:mainfrom
danielmpetrov:bugfix/gh-517

Conversation

@danielmpetrov
Copy link
Copy Markdown
Contributor

Fix #517

@sebastienros
Copy link
Copy Markdown
Owner

It doesn't feel natural that a Date created from a TimeSpan is based on UTC but one created from a Number is based on the culture's TZ. Checking on Shopify's Liquid and other implementations to understand.

@danielmpetrov
Copy link
Copy Markdown
Contributor Author

Exactly. I personally would expect all of them to respect the culture set by the user.

@sebastienros
Copy link
Copy Markdown
Owner

Go with that (from what I am seeing right now). The truth is in the Number to Date conversion, seems like this in Ruby (Time.at(1)) and in LiquidJS. I assume TimeSpan should follow the same logic, only converting the TimeSpan to a number of seconds.

@danielmpetrov
Copy link
Copy Markdown
Contributor Author

This implementation matches the output of LiquidJS, the only problem is I lost the milliseconds somehow. 😂

@danielmpetrov danielmpetrov marked this pull request as ready for review October 20, 2022 18:03
@danielmpetrov
Copy link
Copy Markdown
Contributor Author

OK, I think this does it... let me know 👍

@danielmpetrov danielmpetrov changed the title Force UTC when converting from TimeSpan Update TimeSpan implementation to be consistent with number Oct 20, 2022
@danielmpetrov
Copy link
Copy Markdown
Contributor Author

The problem with this is the string output for time span would be a number. I think we need a TimeSpanValue type, don't we?

@danielmpetrov
Copy link
Copy Markdown
Contributor Author

danielmpetrov commented Oct 20, 2022

I added template tests to verify how a DateTime and TimeSpan object are rendered by default, without any filters. I think these tests are missing and needed, even in the context of these changes.

@danielmpetrov danielmpetrov marked this pull request as draft October 20, 2022 19:06
@danielmpetrov
Copy link
Copy Markdown
Contributor Author

@sebastienros Have you had time to consider how a time span should behave in Fluid? 👀

@sebastienros sebastienros marked this pull request as ready for review November 30, 2022 01:18
@sebastienros
Copy link
Copy Markdown
Owner

@danielmpetrov let me know if you are ok with my changes and I'll merge it.

@sebastienros sebastienros merged commit 0bf05fe into sebastienros:main Nov 30, 2022
@danielmpetrov
Copy link
Copy Markdown
Contributor Author

@sebastienros Looks good to me, and makes sense. Glad this is resolved. 😄

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nondeterministic unit tests

2 participants