Closed devzeb closed 3 years ago
Hi! Thanks for the PR (again)!
I know this will be a pain to do, but could you break this PR down to several small ones since its a mix of different changes? Smaller PRs will help me review them faster so that they are merged quicker.
Regarding your first change, the new changes more closely resemble the Graph API so I think that is a big plus. However, I do have some points for discussion:
completeTask()
vs ModifyTask().setComplete().execute()
). GetTasks()
and CreateTask()
, etc. These look like functions, not classes. What are your thoughts on this?
The second change is a very good idea 👍 . I admit using strings to access properties everywhere in the code was not a good design.
I'm also okay with the third change. I also noticed you added some unit tests. I will add them to CircleCI later.
I'll break the PR up into three smaller ones and comment on the individual PRs then. Sorry for not doing it in the first place.
First change In order to improve the code-useability, I reworked the API calls to use objects instead of functions. This way it's no longer required to add a new function to todo_api.py if a new command should be implemented.
Previously the code looked like:
Now, the new usage is like this:
The new code for example allows us to change multiple properties of a Task:
Second change Objects are returned when querying tasks or lists from the API. Before, a dict was returned with all the attributes from the API response. Now, the API response is converted into an object ("TodoList" or "Task"). The explicit conversion prevents the application breaking through typos when extracting data from a response object.
Third change I reworked the datetime parser to allow the parsing of time strings in "American" format. I updated the help text and README.md accordingly. This includes time expressions like: