-
Notifications
You must be signed in to change notification settings - Fork 22
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
Taskchampion Python bindings #385
Comments
That's fantastic! And, I think this approach looks good: interfacing directly from Python into the A few questions toward finding a way to make progress here;
Maybe a good next step is to get something that works into a PR and get that merged, with multiple TODO's scattered about. That can include README describing the intended implementation, and indicating that it's not yet stable or ready for use. Then we can get a few more issues filed and perhaps a few people can work on separate aspects in parallel. |
To answer your questions:
|
As per your suggestion, I will continue work in the forked repository, so we can possibly PR. This can be pulled into its own, separate repository later on, if need arises. |
Sounds good! |
Ok, I think I have the the core. I'll create PR in a bit, documenting the install/test process. Currently implemented and tested:
Implemented but not tested:
TODO:
|
This is really cool! I will try to take a look, but I've been quite busy so I invite anyone else following along to also jump in and offer feedback. |
I wonder if the new |
There seems to be a few API changes as well, with the deprecations of many Replica related operations, so I'll look into those, and bring up the deprecations up-to-date. |
I am pretty much done with the updates, had to make a few changes due to API updates -
ops = []
task, op = empty_replica.create_task(uuid.uuid4())
ops.append(op)
op = task.set_status(Status.Pending)
ops.append(op)
....
empty_replica.commit_operations(ops)
tasks = empty_replica.all_task_uuids()
assert len(tasks) == 1 The only issue i have ran into so far - my previously written tests for the @pytest.fixture
def working_set(tmp_path: Path):
r = Replica(str(tmp_path), True)
ops = []
result = r.create_task(str(uuid.uuid4()))
assert result is not None
task, op = result
ops.append(op)
result = r.create_task(str(uuid.uuid4()))
assert result is not None
task, op = result
ops.append(op)
r.commit_operations(ops)
return r.working_set() # For whatever reason this working set is empty, even though `r.all_task_uuids()` is not, and contains valid UUIDs. Out of desperation, I've tried to
|
That's fantastic! I am glad to hear the new API worked well.
This sounds like a great change. That's basically what the Rust code is doing, just in a more ergonomic way in that language. Speaking of ergonomics, consider having (task, op) = r.create_task(...)
assert task is not None
...
I suspect this is because the created tasks have no status - they are just a uuid with no properties. I suspect adding ops.append(task.set_status(Status.Pending)) after each
The story is: when I started on all of this, my thinking was that Taskchampion should have a single C interface, and all other languages would integrate against that. It turns out that's not really "how it's done", now that I've had a chance to see some other Rust FFI. Instead, most languages integrate directly with the Rust, even if there's some C involved in the implementation of that integration. And, for the most part C++ applications tend to build their own integrations that are specific to how they use the Rust library, while Python applications tend to use a Python package wrapping the Rust library. So, the C bindings went away, and Taskwarrior now has its own C++ bindings (using https://cxx.rs) that are specific to how it wants to work with Taskchampion. Notably, it doesn't wrap the I don't have a strong opinion on whether the Python wrapper should be in this repo or not. A few thoughts:
What would you prefer? |
That a good suggestion, the only problem is the Python's error handing - if an error occurs, let's say, during UUID parsing, the Python raises a RuntimeException. So there wouldn't be a return value to work with, as the program wound just quit on an error. The signature of these functions is still not correct however - I foolishly copied over Rust signatures, forgetting about the raising of errors. Removing python's # before
def create_task(uuid: str) -> Optional[tuple[Task, Operation]]: ...
# after
def create_task(uuid: str) -> tuple[Task, Operation]: ... I found the issue with my tests and partially my code - the # before
def set_status(status: Status) -> Operation: ...
# after
def set_status(status: Status) -> list[Operation]: ... Making this change and extending the ops list solves the issue with the tests. This is something I was afraid of when implementing the original return-the-operation approach - I wasn't sure if there was a situation where a single method call inserted multiple Operations instead of just one. But this should still be an easy fix, as all I have to do is return the entire list instead of a single Operation. I started this project from a fork, because it was already being done with the C library. But there really is no reason to couple this wrapper with the Taskchampion as all it's doing is providing a convenience wrapper to public structs/methods - the same can be accomplished by using it as a dependency. |
Oh, good find! Just to be sure: that will be necessary for all modifications, not just Regarding the repository: would you like me to create a new repo in this org? Perhaps |
Yea, that was my plan all along.
Feel free to! |
I've added you with write access to empty repo https://github.com/GothenburgBitFactory/taskchampion-py/. Feel free to push to that! Please have a look at this repo for the necessary metadata files (license, code of conduct, etc.). |
Hello,
Really appreciate your work on taskwarrior/taskchampion!
With the release of a new way to interact with the app (via the rust library), python bindings are needed. It's a big request, so I come bearing gifts - a fork with a POC barebones attempt to connect with an existing database instance and task creation here.
It's very much a barebones POC, with two methods, to get the impression of the workload needed; right now the code lives in the
py-lib
directory, and can be assembled via maturin.Here's the demo in python console as an easy test:
Guess I am volunteering to work on this?
The text was updated successfully, but these errors were encountered: