Skip to content

Implement isDaylightSavingTime#1083

Merged
alfonsogarciacaro merged 1 commit intofable-compiler:masterfrom
inosik:api/DateTime.IsDaylightSavingTime
Jul 18, 2017
Merged

Implement isDaylightSavingTime#1083
alfonsogarciacaro merged 1 commit intofable-compiler:masterfrom
inosik:api/DateTime.IsDaylightSavingTime

Conversation

@inosik
Copy link
Copy Markdown
Contributor

@inosik inosik commented Jul 18, 2017

References #1082.

Because JS doesn't have a native function for this, we need to check if the Dates UTC offset is equal to either the offset in January or in June.

If a users time zone doesn't have DST, this function should always return false.

@alfonsogarciacaro where can I add tests for the isDST function?

References fable-compiler#1082.

Because JS doesn't have a native function for this, we need to check if the
`Date`s UTC offset is equal to either the offset in January or in June.

If a users time zone doesn't have DST, this function should always return
`false`.
@alfonsogarciacaro
Copy link
Copy Markdown
Member

Thanks a lot for this! @forki's test should be enough to check isDaylightSavingTime, do you want to test isDST separately?

@alfonsogarciacaro alfonsogarciacaro merged commit c383c2e into fable-compiler:master Jul 18, 2017
@inosik
Copy link
Copy Markdown
Contributor Author

inosik commented Jul 18, 2017

I actually wanted to test it with some predefined values, but maybe it's just me being paranoid. 😄

In this case, there's probably no reason to make it a separate function.

@inosik inosik deleted the api/DateTime.IsDaylightSavingTime branch July 18, 2017 10:55
inosik added a commit to inosik/Fable that referenced this pull request Jul 18, 2017
@forki
Copy link
Copy Markdown
Collaborator

forki commented Jul 18, 2017 via email

@forki
Copy link
Copy Markdown
Collaborator

forki commented Jul 18, 2017

@inosik thanks for implementing it.
you win a music video: https://www.youtube.com/watch?v=7aJYK3PnWqU

@inosik
Copy link
Copy Markdown
Contributor Author

inosik commented Jul 18, 2017

Sweet! Now I need to win some speakers for my workstation 😄

@alfonsogarciacaro
Copy link
Copy Markdown
Member

This is what I see most of the times when I try to share a video between Germany and Spain ;)

screen shot 2017-07-18 at 1 42 07 pm

@forki
Copy link
Copy Markdown
Collaborator

forki commented Jul 18, 2017

sorry guys you are missing out.

@inosik
Copy link
Copy Markdown
Contributor Author

inosik commented Jul 18, 2017

Unfortunately, the fix breaks Travis CI. AppVeyor seems to be completely broken right now.

@alfonsogarciacaro
Copy link
Copy Markdown
Member

Luckily my advanced hacker skills prevail over Youtube algorithm... by typing the video title in the Search Bar ;)

@alfonsogarciacaro
Copy link
Copy Markdown
Member

I hate Travis, it's been failing continuously for a while now for not apparent reason 😬

@inosik
Copy link
Copy Markdown
Contributor Author

inosik commented Jul 18, 2017

AppVeyor doesn't seem to run at all at the moment, but Travis fails because of the test from #1082.

@inosik
Copy link
Copy Markdown
Contributor Author

inosik commented Jul 18, 2017

// index.js
function isDaylightSavingTime(x) {
    var jan = new Date(x.getTimezoneOffset(), 0, 1);
    var jul = new Date(x.getTimezoneOffset(), 6, 1);
    return isDST(jan.getTimezoneOffset(), jul.getTimezoneOffset(), x.getTimezoneOffset());
}

function isDST(janOffset, julOffset, tOffset) {
    return Math.min(janOffset, julOffset) === tOffset;
}

var d1 = new Date(2017, 6, 18, 2, 0, 0);
var d2 = new Date(2017, 11, 18, 2, 0, 0);

console.log(d1.toString() + " is DST: " + isDaylightSavingTime(d1).toString());
console.log(d2.toString() + " is DST: " + isDaylightSavingTime(d2).toString());
$ node index.js
Tue Jul 18 2017 00:00:00 GMT+0200 (Mitteleuropäische Sommerzeit) is DST: true
Mon Dec 18 2017 00:00:00 GMT+0100 (Mitteleuropäische Zeit) is DST: false

Works for me. Must be the time zone of the Travis instance.

@alfonsogarciacaro
Copy link
Copy Markdown
Member

Let's see what Appveyor says and, if necessary, disable that test in the CI servers.

@inosik
Copy link
Copy Markdown
Contributor Author

inosik commented Jul 18, 2017

As I wrote in #1082, this test will fail for everybody who runs it in a country without DST. That's why I wanted to test the isDST function instead of DateTime.IsDaylightSavingTime().

Looks like we can control the time zone in Travis CI with the TZ environment variable and in AppVeyor with tzutil, which is a Windows program. But I don't think that it's a good idea to rely on this.

@forki
Copy link
Copy Markdown
Collaborator

forki commented Jul 18, 2017

can ask for the current zone? we might then just enable US, Germany and that's it

fable-repo-assist Bot added a commit that referenced this pull request Apr 26, 2026
Fix code scanning alerts IONIDE-009 (alerts #1056-#1058, #1069-#1070, #1083)
and IONIDE-002 (alerts #1065, #1072, #1181).

- Add [<return: Struct>] to partial active patterns in BabelPrinter.fs and
  Python transforms to avoid heap allocation for option types
- Replace [] postfix array syntax with the idiomatic 'array' postfix syntax
  in BabelPrinter.fs, Fable2Python.Util.fs, and Python.AST.fs

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

3 participants