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

Change feeds to use local datetime #69

Merged
merged 3 commits into from
Oct 7, 2018

Conversation

richardhozak
Copy link
Contributor

Internally the Feed struct uses Utc datetimes converted from local datetimes.
Datetime stored in feed files are always stored as local.

The basic process is:

  • Feed is read from feed file with local datetime (FeedEvent::read).
  • This is then converted to Feed struct which internally stores datetime as Utc
  • When we want to know if feed is scheduled, we find the duration from Utc datetime to Local datetime (this is handled by chrono).

I must say the test_parse_events test, gave me pretty hard time to fix, I could not find simple way to get the offset from Utc datetime to Local datetime in chrono crate, but it works now.

Fixes #58

@porglezomp
Copy link
Owner

I'm not super experienced with date handling code, so I don't have a super strong opinion on it, but can you explain the decision to store time as local times and do math with UTC?

Also, will this tolerate the switch-over? I suspect the parse() call on the DateTime<Local> will handle it, but that's also worth double-checking.

@porglezomp porglezomp self-requested a review October 3, 2018 19:32
When we check if comic is scheduled, we convert parsed datetime
to localtime and check elapsed time against that.
@richardhozak
Copy link
Contributor Author

Now that I think of it, it is much easier to parse datetime as Utc, as it was before, and when we check if comic is scheduled, we just convert this Utc datetime to our local time and check elapsed time against that.

It is much better solution, I don't know why I didn't think of that earlier.

Changed the commit to reflect latest changes.

Copy link
Owner

@porglezomp porglezomp left a comment

Choose a reason for hiding this comment

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

Wow, this became much simpler.

I think there are a few more changes in is_scheduled that need to be made in order to avoid some edge cases that weren't properly handled previously though, and it'd be good to fix those here.

src/feed.rs Outdated Show resolved Hide resolved
src/feed.rs Outdated
@@ -157,7 +157,8 @@ impl Feed {
}

pub fn is_scheduled(&self) -> bool {
let elapsed_time = Utc::now().signed_duration_since(self.last_read);
let last_read = self.last_read.with_timezone(&Local);
let elapsed_time = Local::now().signed_duration_since(last_read);
Copy link
Owner

Choose a reason for hiding this comment

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

The On case probably needs to loop from the last_read's day all the way to the now day, rather than using elapsed_time.num_days(), to account for incomplete days.

@porglezomp
Copy link
Owner

Exploring the semantics of the is_scheduled policies:

@ every # day(s) should wait at least # days. I think this should be independent of the times during those days, so if you read it at midnight on one day, and the morning a week later, less than 7 days have elapsed, but it should be considered as 7 days. I think this one is handled incorrectly, and we need to count days of the week rather than number of days in the elapsed time.

@ on monday/tuesday/etc should require that it was last read before that day, and it is now that day or later. Currently, if you last read it sunday night, and it's marked as @ on monday, I believe it will currently think 0 days have passed, and so not realize that you went from sunday -> monday.

If you could add test cases for these two edge cases, that would also be very much appreciated.

@porglezomp
Copy link
Owner

Since it makes it easier to test, I'd also approve of changing is_scheduled to accept the "current" time as a parameter.

@richardhozak
Copy link
Contributor Author

Ok, I changed the is_scheduled to accept datetime, I also changed how is determined if the feed has already been read at least one. Before you would store MIN_DATE if you did not read it at least once, now it is Option with Some(DateTime) or None if the feed has not been read.

@porglezomp
Copy link
Owner

Do you want to handle those other edge condition issues I brought up, or do you want me to make that a separate issue? This is a good improvement already, so it's up to you.

@richardhozak
Copy link
Contributor Author

Yeah, maybe it'll be better to open separate issue. We can then discuss how to handle these two edge cases properly.

@porglezomp
Copy link
Owner

Okay, in that case, you need to re-run rustfmt on this so Travis will accept it.

Copy link
Owner

@porglezomp porglezomp left a comment

Choose a reason for hiding this comment

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

LGTM

@porglezomp porglezomp merged commit 64dedbb into porglezomp:develop Oct 7, 2018
@porglezomp
Copy link
Owner

Merged, thanks for your fix!

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.

"@ on date" doesn't use the local time
3 participants