Refactor #60
Refactor #60
Conversation
|
Thanks for the PR I'll look into it soon and if all looks good I'll merge it next week :) |
There was a problem hiding this comment.
First of all - Thank You.
I really liked what you did here, it's already better than what it was previously.
I did leave a few notes that I think could make this project more maintainable and more professional.
If you don't have the time/don't want to implement those notes, that's understandable, my bad for taking so long with the CR.
Anyway I would be glad if you could let me know if you're up for it, if you're not interested I'll take it from here when I get the chance :)
i3_agenda/event_utils.py
Outdated
| @@ -0,0 +1,49 @@ | |||
|
|
|||
There was a problem hiding this comment.
it would be better, instead of having a "utils" file, to have an EventFetcher class, with the get_closest method and get_events method
There was a problem hiding this comment.
Well, I agree that there is a better solution that this "utils" file, but just to understand it better:
- is_allday could be a Event method
- get_event_time, as commented bellow, and get_closest I see as functions, so I propose to these functions only be moved to the event.py file as separated functions
There was a problem hiding this comment.
What I meant here is that instead of having the get_closest and get_events as functions, we could have an object called EventFetcher and add those functions as its methods
|
Did some changes, I think most of them it is better, the reply of the reviews will help in next improvements. And yes I am up with this yet, just give some feedbacks and I can address it. |
Great! I left some comments, let me know what you think. |
|
Thanks again very much for the changes, you did a great job on pretty much everything |
|
Owww man, I thought here and I realised that is possible to infer if the event is all day looking if it begins at midnight of a day and ends at the midnight of the next day. It looks to be working. It solves a lot of problems discussed before. But I changed the name of the variable unix_time to start_time, and it continue to be a float field. Well but this change of the name of the attribute broke the cache, so it needs a treatment |
|
Looking good! I think we juts need to figure out the all_day thing and iron out some minor stuff, then we can merge it 😀 |
|
Great! |
This comment has been minimized.
This comment has been minimized.
| ) | ||
| ) | ||
| except KeyError: | ||
| return None |
There was a problem hiding this comment.
If one event couldn't be parsed correctly, should we drop the entire cache or continue trying?
I think dropping the entire cache is fine, just needs a comment saying "One of the items are invalid. Cache must be invalid"
|
|
||
| def is_allday(self) -> bool: | ||
| time_delta = self.end_time - self.start_time | ||
| return self.get_datetime().time() == dt.time(0) and time_delta % 24 == 0 |
There was a problem hiding this comment.
probably worth adding a comment on this one since it's not super clear what we're doing here
This PR Fixes #56 and fixes #58
Refactor the code
it would be great to have some feedback in this part of PR
the event is urgent if it begins in five minutes and if it hasn't passed 5 minutes it started
Sorry by the large commit, I was changing a lot and all of this is a requirement to the others.