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

Late sunsets give incorrect date for time zone #26

Closed
tal opened this issue Jul 2, 2017 · 1 comment
Closed

Late sunsets give incorrect date for time zone #26

tal opened this issue Jul 2, 2017 · 1 comment
Labels

Comments

@tal
Copy link

tal commented Jul 2, 2017

I'm getting an issue with putting in a date that is after midnight UTC but before midnight local. It returns a solar instance for the following day. I've tried passing in a custom calendar but that doesn't seem to fix the issue. I'll continue working on it but for now here's a test I wrote to recreate this failure case.

	func testLateTimeCorrectReturnDate() {
		let lateTime = Date(timeIntervalSince1970: 1499130000) // After midnight UTC, before midnight in DC.
		let city = cities.first(where: { $0.name == "Washington, D. C." })!
		
		let calendar: Calendar = {
			var calendar = Calendar.current
			calendar.timeZone = TimeZone(identifier: "EST")!
			return calendar
		}()
		
		guard let solar = Solar(for: lateTime, latitude: city.latitude, longitude: city.longitude) else {
			XCTFail("Cannot get solar")
			return
		}
		
		let sameDate = calendar.compare(lateTime, to: solar.sunset!, toGranularity: .day)
		
		XCTAssertTrue(sameDate == .orderedSame, "The sunset given \(solar.sunset!) is not same day as given date \(lateTime)")
	}
@tal tal changed the title Late sunsets give incorrect date for Late sunsets give incorrect date Aug 5, 2017
@tal tal changed the title Late sunsets give incorrect date Late sunsets give incorrect date for time zone Aug 5, 2017
tal added a commit to tal/Solar that referenced this issue Aug 6, 2017
Fixes problem described in ceeK#26.
@ceeK ceeK added the bug label Aug 8, 2017
@ceeK
Copy link
Owner

ceeK commented Aug 8, 2017

Hi Tal! Thanks for the report and including a test, super helpful! Apologies for the delay.

I've had a look at your test and as far as I can tell it seems to be working as expected, in that it always returns a UTC date that you have to format 🤔 maybe I am misunderstanding though. I've added a little debug code to the bottom of your test. Can you check my logic:

func testLateTimeCorrectReturnDate() {
        let lateTime = Date(timeIntervalSince1970: 1499130000) // Tuesday, 4 July 2017 01:00:00 UTC
        let city = cities.first(where: { $0.name == "Washington, D. C." })!
        
        let calendar: Calendar = {
            var calendar = Calendar.current
            calendar.timeZone = TimeZone(identifier: "EST")!
            return calendar
        }()
        
        guard let solar = Solar(for: lateTime, latitude: city.latitude, longitude: city.longitude) else {
            XCTFail("Cannot get solar")
            return
        }
        
        let sameDate = calendar.compare(lateTime, to: solar.sunset!, toGranularity: .day)
        
        XCTAssertTrue(sameDate == .orderedSame, "The sunset given \(solar.sunset!) is not same day as given date \(lateTime)")
        
        let dateFormatter = DateFormatter()
        dateFormatter.dateStyle = .full
        dateFormatter.timeStyle = .full
        dateFormatter.timeZone = TimeZone(identifier: "EST")!
        let string = dateFormatter.string(from: solar.sunset!)
        print("Date: \(string)")
    }

This outputs Date: Tuesday, July 4, 2017 at 7:36:56 PM GMT-05:00. I've had a look this reference website and can see that the local sunset is July 4th 8:36pm GMT-04:00 (EDT). This looks to me that it's producing the correct sunset date given the 1 hour DST difference between EST and EDT.

After running your pull request test and checking Solar's date, I get:
2017-07-04 00:37:07 +0000. The timezone here is UTC, so when we format this date to EST we actually get a date on 2017-07-03.

Tell me if I'm missing something! 🙈

@ceeK ceeK closed this as completed Oct 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants