-
Notifications
You must be signed in to change notification settings - Fork 16
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
Added complete function, reworked link function and added tests #121
Conversation
Added the complete function to programmatically complete a things task/project using the Things URL Scheme. Modified the link function to a more general version - Changed hardcoded "show" action to a variable - every link now contains the auth_token query argument - additional query arguments can now be supplied using **kwargs Modified the show function to account for the updated link function Added test for complete function and modified tests for link function to account for the updated link function
That's nice! I always wanted to add support for the URL Scheme. Thank you for the patch. Couldn't find time to review, yet. |
@cato447 Thank you! Two suggestions, otherwise looks good to me:
Here's some suggested code (you might find other actions that don't require a token):
Also, I wonder if we could create an alias |
My results from testing: |
link implementation respects old usage of link now link function only adds authentication if needed added url as an alias for link as things documentation only mentions things urls tests updated accordingly
Good point, I see what you're saying. How about doing it like this:
That way you can do Documentation: you might want to specify the parameters in the documentation (and that kwargs are the other query params). See the Also, I don't know what you think about this, but I'm tempted for the main method to be called |
Calling the main method url instead of link is a good idea. I tested a little bit a more and found another problem. My first ideas would be to pass the parameters as a dictionary and not as variables in kwargs |
… url url is now the main way to generate things urls. link is kept as an alias for backward compatibility. things url generated by url is now url encoded to match with the things url scheme documentation. renamed action variable to command in url to better match with things url scheme documentation. added a test covering url encoding
👍 This sounds good to me. However, keeping kwargs might work as well. def f(**kwargs):
print(kwargs)
d = {"what-is-this": 10} Now you can do this: >>> f(**d)
{'what-is-this': 10} |
things/api.py
Outdated
>>> things.url(command="add", title="new task", when="in 3 days", deadline="in 6 days") | ||
'things:///add?title=new%20task&when=in%203%20days&deadline=in%206%20days' | ||
""" | ||
query_params = dict(**kwargs) if uuid is None else dict(id=uuid, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't want to mutate kwargs
, then you can do dict(kwargs)
as well. (Since kwargs
is created afresh on every call, I wouldn't think there's any danger in mutating it directly either... or is it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not that experienced using kwargs
. I just copied the statement from the else case to get it to work.
There is no deeper though behind using dict(**kwargs)
@cato447 Great work! This looks good to me besides the comments. Thank you for your work here and sorry about the thorough review. |
Implemented requests from review (documentation improvements). Added test case to tests and doctest which covers passing a dictionary of parameters to url. This is needed if a parameter is in kebab-style which is not valid python syntax for a variable name.
No problem! This is currently part of my procrastination project and I am very thankful for your work and am happy to see this project being well maintained :D |
@cato447 I made some minor tweaks to |
@mikez looks good to me. Thank you for your time! |
fixed suggestions via lint
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
===========================================
- Coverage 100.00% 99.34% -0.66%
===========================================
Files 2 3 +1
Lines 363 455 +92
===========================================
+ Hits 363 452 +89
- Misses 0 3 +3 ☔ View full report in Codecov by Sentry. |
@cato447 Lint is reporting two "line too long" errors:
Sorry about this, that lint is very strict. Can you fix those? I'll merge in later today. |
What should I do about the other errors in the lint report? |
@cato447 Anything that doesn't involve code that you wrote, I will fix. |
add pylint direction
|
FWIW, I opened up an issue on the pylint error here: pylint-dev/pylint#9715 and pyright error here: microsoft/pyright#8107 |
Thanks @cato447. Congrats on the merge! 🥳🎉 |
Motivation
I am working on a project that syncs Things3 with reclaim.ai.
For this I needed a way to programmatically complete a things task.
Changes made:
Added the complete function to programmatically complete a things task/project using the Things URL Scheme.
Modified the link function to a more general version
Modified the show function to account for the updated link function
Added test for complete function and modified tests for link function to account for the updated link function