Feat: adding config in CLI #39
Conversation
…class to call create tasks
…els from the repo
Reviewer's Guide by SourceryThis pull request adds configuration functionality to the CLI and introduces several new files and modifications to existing ones. The changes primarily focus on implementing a configuration system, enhancing the CLI structure, and adding new utility functions. Key additions include a new main.rs file for the CLI, modifications to the TES module, and the introduction of configuration and transport-related files. File-Level Changes
Tips
|
|
accidently made pr with wrong branch |
![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 - here's some feedback:
Overall Comments:
- Consider replacing
unwrap()calls in the CLI implementation with more robust error handling to improve the user experience when invalid input is provided. - There's some code duplication in the CLI command handling. Consider refactoring common patterns into shared functions to improve maintainability.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 3 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| Ok(config) | ||
| } | ||
|
|
||
| fn load_configuration() -> Configuration { |
There was a problem hiding this comment.
suggestion: Improve error handling in configuration loading
The current implementation of load_configuration() doesn't handle potential errors very gracefully. Consider refactoring to provide more informative error messages and handle failure cases explicitly.
fn load_configuration() -> Result<Configuration, ConfigError> {
let config_file_path = dirs::home_dir()
.ok_or(ConfigError::HomeDirNotFound)?
.join(".config/config.json");
| endpoint: &str, | ||
| data: Option<Value>, | ||
| params: Option<Value>, | ||
| ) -> Result<String, Box<dyn Error>> { |
There was a problem hiding this comment.
suggestion: Consider using custom error types instead of Box
Using Box is quite generic. Consider creating custom error types to provide more specific and informative error handling throughout the application.
) -> Result<String, TransportError> {
// ... (elsewhere in the file)
#[derive(Debug, thiserror::Error)]
pub enum TransportError {
#[error("URL parsing error: {0}")]
UrlParseError(#[from] url::ParseError),
#[error("Request error: {0}")]
RequestError(#[from] reqwest::Error),
// Add other specific error types as needed
}
| @@ -0,0 +1,36 @@ | |||
| # A Generic SDK and CLI for GA4GH API services | |||
There was a problem hiding this comment.
suggestion (documentation): Consider capitalizing 'Services' for consistency.
To maintain a consistent title case format, consider changing 'services' to 'Services'.
| ``` | ||
| cargo nextest run | ||
| ``` | ||
| For checking the unit converage, you can run: |
There was a problem hiding this comment.
issue (documentation): Typo in the word 'converage'.
The word 'converage' should be corrected to 'coverage'.
| ``` | ||
| bash ./run-tests.sh | ||
| ``` | ||
| or, you can run using cargo nextest using |
There was a problem hiding this comment.
issue (documentation): Repetition of the word 'using'.
The phrase 'using cargo nextest using' should be rephrased to 'using cargo nextest'.
Here, most of the code is copied #38, only getting config is added. Please merge this after #38.
Summary by Sourcery
Introduce a CLI tool for managing tasks using the TES API, add configuration support, implement TES and transport modules, and set up CI/CD workflows and documentation.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests: