Skip to content
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

WIP: Taskchampion bindings for python #388

Closed
wants to merge 21 commits into from

Conversation

illya-laifu
Copy link

Taskchampion bindings for python

Installation

The dependency management is done via poetry, to install the dependencies, from the py-lib run:

poetry install

To build and install the package into current environment:

poetry run maturin develop

To run tests:

poetry run pytest

Currently implemented and tested:

  • Annotation class and the getters/setters for the two fields.
  • Replica class (::dependency_map method needs more testing and polishing due to the Rc reference)
  • Status enum
  • Tag class
  • Task class *
  • WorkingSet class

Implemented but not tested:

  • DependencyMap class
  • Storage concrete classes.

TODO:

  • Be able to construct replica by passing in the specific Storage implementations (e.g. InMemoryStorage, SqliteStorage or users own implementation of Storage)
  • Current Task class is the immutable version of it, it would be nice to extend it to also include the methods from TaskMut and convert to and the mutable state in those method calls. It is impossible to implement the TaskMut via pyo3 due to the lifetime parameters.*
  • Be able to iterate over WorkingSet via next() and with for item in ws:, but this needs it to hold the reference to an iterator.
  • Python has it's own uuid library, currently the methods that accept a UUID, do that by accepting str and converting it into the rust's implementation of UUID.
  • Better documentation, but the Rust comments on the pyo3 annotated functions/objects directly translate into the docstrings.
  • Not entirely sure if it is possible (need to research), but it would be convenient to provide signature stub files along with the library, so the Language Servers, linters and such would be able to infer the types.
  • Either split this into a separate repo, since maturin generates multiple github workflows, or rename the py-lib into something sensible.

Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know Py03 well enough to really review this in detail, but structurally this looks great!

When this is ready for review, I will try to learn enough to verify

  • it does what it says on the tin
  • it does not make any promises to users that we cannot keep (e.g., making an assumption that TaskChampion doesn't actually support)
  • it seems safe :)

I have a minor concern that refactors of TC (notably #372) will cause breaking changes to bindings like this. However, I think that's OK at this stage -- we can make a major version release of TC, with well-documented required changes, and consumers can upgrade as necessary.

Comment on lines +7 to +9
- There is no good way to describe functions that accept interface (e.g. `Replica::new` accepts any of the storage implementations, but Python bindings lack such mechanisms), currently, `Replica::new` just constructs the SqliteStorage from the params passed into the constructor.
- Currently Task class is just a reflection of the rust's `Task` struct, but constructing the standalone `TaskMut` is impossible, as Pyo3 bindings do not allow lifetimes (python has no alternatives to them). Would be nice to expand the `Task` class to include the methods from `TaskMut` and convert into the mutable state and back when they are called.
- It is possible to convert `WorkingSet` into a python iterator (you can iterate over it via `for item in <blah>:` or `next(<blah>)`), but that needs a way to store the current state.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three of these are valid criticisms of the Rust API, and problematic for embedding in other languages, too. I'd be happy to see PRs to simplify these Rust APIs. In fact, #372 is one such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants