Adding Reminder time #133
Adding Reminder time #133
Conversation
…. This includes a new convert_thingstime_sql_expression_to_isotime() function, additions to the make_tasks_sql_query function, and adding REMINDER_TIME global variable.
There was a problem hiding this comment.
@HiJohnElliott This looks great to me in terms of functionality! 🎉🎉🎉
All outstanding remarks relate to documentation and tinsy things to make the life of those easier who view the code in the future. Let me know if what I wrote made sense.
|
@AlexanderWillner Heads-up: as soon as this review is done, we might want to push a new release (also considering the other recent update); would this be something you'd enjoy to do? |
|
Hey @mikez! I would LOVE to be a part of pushing the new release once we have merged the changes. Sounds fun! |
Has been a while we pushed a release ;) Given that there was no major (negative) feedback, I'd like to suggest following Semantic Versioning and call the next release 1.0.0. Depending on the next changes we would push 1.0.1 or 1.1.0. |
|
@AlexanderWillner Haha, yes what the heck, let's do a 1.0.0! Maybe we could have a celebration. 😊🎉 |
@HiJohnElliott Pushing a release is something only we admins can do at this point; however, if you'd enjoy contributing more once this is done, you could check out this open release. |
… the previous pull request as well as an updated version of the test_thingstime() function that only checks a single task in the test DB. The task with uuid 7F4vqUNiTvGKaCUfv5pqYG has been updated in the test .thingsdatabase files to include a reminder_time value that decodes as 12:34:00. The updated databse files are included in this commit as wel.
|
Hey @mikez! I just pushed a new commit that includes changes to the items we have discussed here. Tests are still passing and if anything else needs tidying up just let me know and I'll be happy to tend to it. I'd also be more than happy to take a look at that issue you linked to relating to repeatable to-dos showing up in upcoming() once we are on the other side of this PR and release. Thanks! |
Although I notice issues and comments in my Things inbox, I choose not to act on them immediately, especially since I don’t anticipate much activity in this repository. |
…tasks_sql_query() where they were wrapping the deadline, start_date, and reminder_time expressions. FIX: This commit also fixes the missing prefixed zero in the description of the reminderTime and the incorrect function name in the comment on line 111.
|
Fixes pushed @mikez! |
…ixes the input and output in the same description. It is also includes a change in test_things.py to the function such that things.get function is called and the 'reminder_time' on the next line instead of at the moment the function is called.
|
@HiJohnElliott Thank you for your patience and endurance here! I hope this code review didn't take away too much of the fun of coding. |
|
I can't thank you enough for working with me on this PR @mikez and apologies that it has taken up so much of your attention. This next push should contain the fixes you asked for. |
… the correct inputs and outputs in the examples and also fits better stylistically. It also includes a change to to ensure that key values are handled correctly in instances where the task has no reminder_time key.
- use `.tasks` instead of `.get` - missing newline - use "black"-style `make auto-style` double-quotes instead of single-quotes
|
|
@HiJohnElliott I made some minor tweaks to the code. In essence it boils down to running these commands: to ensure consistent style according to black; and which checks for trailing whitespace and the like. The final thing I'd like you to do is to squash all your commits (including my last two ones) into one commit which contains all the changes. That way, the version history stays clean for the main repo. |
|
@HiJohnElliott It seems GitHub now has functionality to do the squash automatically, so I used it. :) |
|
@AlexanderWillner Feel free to push a new release as discussed. Also, take a look at the tracemalloc comment, in case that needs to be removed. |
|
@mikez I truly can't thank you enough for being so generous with your time and working with me to get this PR through. I do Tier 2 technical support at a software company here in the US so while I frequently write a lot of SQL and little Python tools here and there, this was the first time I actually had to work with an awesome testing library, with pull requests, with a handful of other topics that were new to me. I know this took a lot of your time but I learned a ton and I couldn't be more excited to have this new reminder_time functionality out in the open for folks to use. One last thing to confirm: now that this branch has been pulled in, am I safe to delete it? I want to make sure that nothing important is lost. |
|
@HiJohnElliott Yep, you're fine to delete it. It was a pleasure guiding you. First PRs, from experience, are always a bit more involved. :) |
Greetings!
This PR adds the reminder time value to any tasks in the app that have one. The reminder time value is returned as an ISO 8601 formatted time string in the hh:mm:ss style and is in 24-hour time.
This PR includes a function called
convert_thingstime_sql_expression_to_isotime()that returns a sql expression that decodes the reminderTime integer value in the database in thereminderTimecolumn. It is written to matchconvert_thingsdate_sql_expression_to_isotime()in function and style as closely as possible.Just like other values such as "trashed', 'deadline", and "tags", reminder_time is ignored and not returned if the task has no reminderTime value.
This PR also includes a test added to
test_things.pythat verifies the sql expression being returned by the new function. The new functionality and test appear to be passing.This is also my first PR so if there is more that I can do to help please let me know as I am sure that I missed something.
j Ξ