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

MRG: refactor the internal Rust API to support container-level functionality #569

Merged
merged 43 commits into from
Jan 7, 2025

Conversation

ctb
Copy link
Collaborator

@ctb ctb commented Jan 5, 2025

This PR splits out an internal Rust API into new _obj functions for many of the search commands in the branchwater plugin, to provide better separation between the CLI and the internal Rust API. The ultimate goal is to build out an intermediate container-level Python API in #552, but that was getting messy between the Rust changes, pyo3 layer additions, and Python tests! Hence, this PR, which will probably involve minimal changes to the CLI and the tests. See #552 for some of the cool new stuff this enables.

This is all refactoring, no new functionality.

A few other features of note - this PR

  • sequesters most of the println and eprintln progress & error reporting to the CLI-level stuff
  • switches rare/unexpected errors over to expect instead of raising an explicit Err
  • removes all of the Box<dyn std::error::Error> stuff, and manages it via a mixture of anyhow and ? conversion
  • changes the container-level layer to report useful numbers (n_processed etc) while keeping the CLI layer focused on reporting exit codes

TODO:

  • remove out all the @CTB notes
  • sequester the new clones out

@ctb ctb mentioned this pull request Jan 5, 2025
1 task
Base automatically changed from refactor_gather_csv to main January 7, 2025 15:12
@ctb ctb changed the title WIP: refactor the internal Rust API to support container-level functionality MRG: refactor the internal Rust API to support container-level functionality Jan 7, 2025
@ctb
Copy link
Collaborator Author

ctb commented Jan 7, 2025

@bluegenes ready for review!

Copy link
Contributor

@bluegenes bluegenes left a comment

Choose a reason for hiding this comment

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

Nice. The additional layer of abstraction seems pretty clean here, excited for what it can enable 🎉

@ctb ctb merged commit 99c0a87 into main Jan 7, 2025
3 checks passed
@ctb ctb deleted the factor_out_rust_api branch January 7, 2025 16:00
@ctb
Copy link
Collaborator Author

ctb commented Jan 7, 2025

cleanliness FTW! we've learned so much 😆

@ctb ctb mentioned this pull request Jan 15, 2025
1 task
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