Feat: TES struct #36
Feat: TES struct #36
Conversation
…class to call create tasks
…els from the repo
Reviewer's Guide by SourceryThis pull request introduces a new TES (Task Execution Service) struct along with its associated models and functionality. The changes include the implementation of the TES struct, configuration settings, transport layer, and various models required for the TES API. Additionally, it sets up the project structure, adds build scripts, test utilities, and CI/CD workflows. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @aaravm - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 2 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| fn serialize_to_json<T: Serialize>(item: T) -> Value { | ||
| serde_json::to_value(&item).unwrap() |
There was a problem hiding this comment.
suggestion: Consider handling serialization errors more gracefully
Instead of using unwrap(), return a Result to allow proper error handling by the caller. This prevents potential panics and improves the robustness of the library.
| fn serialize_to_json<T: Serialize>(item: T) -> Value { | |
| serde_json::to_value(&item).unwrap() | |
| fn serialize_to_json<T: Serialize>(item: T) -> Result<Value, serde_json::Error> { | |
| serde_json::to_value(&item) | |
| } |
| pub fn urlencode<T: AsRef<str>>(s: T) -> String { | ||
| ::url::form_urlencoded::byte_serialize(s.as_ref().as_bytes()).collect() |
There was a problem hiding this comment.
suggestion: Update URL encoding method to use a more modern approach
The current method is deprecated. Consider using a more up-to-date approach for URL encoding, such as the percent_encode function from the percent-encoding crate.
pub fn urlencode<T: AsRef<str>>(s: T) -> String {
use percent_encoding::{utf8_percent_encode, NON_ALPHANUMERIC};
utf8_percent_encode(s.as_ref(), NON_ALPHANUMERIC).to_string()
}
| /// # Returns | ||
| /// | ||
| /// A new instance of Configuration. | ||
| pub fn new( |
There was a problem hiding this comment.
suggestion (bug_risk): Add validation for the base_path URL in the Configuration::new method
Adding validation for the base_path URL in the new method would prevent issues down the line. Consider checking if the URL is valid and returning a Result instead of assuming it's always correct.
pub fn new(base_path: Url) -> Result<Self, url::ParseError> {
if !base_path.has_host() {
return Err(url::ParseError::EmptyHost);
}
Ok(Self { base_path })
}
| /// The `Configuration` struct is responsible for specifying details of the Endpoint where the requests are made. | ||
| /// It provides methods for making constructing new configuration, changing the base url, and specifying a default configuration. | ||
| #[derive(Debug, Clone)] | ||
| pub struct Configuration { |
There was a problem hiding this comment.
suggestion: Consider using more specific types for some Configuration fields
Some fields in the Configuration struct are using Option<String>. Consider using more specific types for some of these, like Option<Url> for base_path. This would provide better type safety and make the intended use of these fields clearer.
pub struct Configuration {
/// The base path for API requests.
pub base_path: Url,
pub user_agent: Option<String>,
pub client: reqwest::Client,
pub basic_auth: Option<BasicAuth>,
pub oauth_access_token: Option<String>,
pub bearer_access_token: Option<String>,
}
|
|
||
| ## Running the tests | ||
|
|
||
| Before running the tests, you need to install Funnel, a task execution system that is compatible with the GA4GH TES API. Follow the instructions in the [Funnel Developer's Guide](https://ohsu-comp-bio.github.io/funnel/docs/development/developers/) to install Funnel. |
There was a problem hiding this comment.
suggestion (documentation): Consider adding a brief description of Funnel.
Adding a brief description of Funnel can help users quickly understand what it is without needing to follow the link.
| Before running the tests, you need to install Funnel, a task execution system that is compatible with the GA4GH TES API. Follow the instructions in the [Funnel Developer's Guide](https://ohsu-comp-bio.github.io/funnel/docs/development/developers/) to install Funnel. | |
| Before running the tests, you need to install Funnel. Funnel is a lightweight, open-source task execution system that implements the GA4GH Task Execution Service (TES) API. It allows you to run scientific workflows and batch jobs across various compute environments. Follow the instructions in the [Funnel Developer's Guide](https://ohsu-comp-bio.github.io/funnel/docs/development/developers/) to install Funnel. |
| ``` | ||
| cargo nextest run | ||
| ``` | ||
| For checking the unit coverage, you can run: |
There was a problem hiding this comment.
suggestion (documentation): Consider changing 'unit coverage' to 'unit test coverage'.
Changing 'unit coverage' to 'unit test coverage' can improve clarity.
| For checking the unit coverage, you can run: | |
| For checking the unit test coverage, you can run: |
|
Lots of unnecessary files got added |
This PR contains the TES struct.
Please ignore the files:
as these files will be merged from #26 and #30, and these files are only present to have the appropriate definitions here as well and analyze only the file
lib/src/tes/mod.rsSummary by Sourcery
Introduce TES struct and related models for managing task execution services, implement Transport struct for HTTP requests, add configuration management, and set up CI/CD workflows and tests.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests: