Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| [package] | ||
| name = "coman" | ||
| version = "0.8.9" | ||
| edition = "2024" |
There was a problem hiding this comment.
Version field was removed, ensure this is intentional and that versioning is handled elsewhere.
#ai-review-inline
| env!("VERGEN_BUILD_DATE"), | ||
| ")" | ||
| ); | ||
| const VERSION_MESSAGE: &str = concat!(env!("CARGO_PKG_VERSION"), " (", env!("VERGEN_BUILD_DATE"), ")"); |
There was a problem hiding this comment.
The VERSION_MESSAGE now omits the git describe information which might reduce debuggability.
| const VERSION_MESSAGE: &str = concat!(env!("CARGO_PKG_VERSION"), " (", env!("VERGEN_BUILD_DATE"), ")"); | |
| Consider re-adding the git describe info for better version tracking in production builds. |
#ai-review-inline
| release: | ||
| name: Release - ${{ matrix.platform.os_name }} | ||
| env: | ||
| COMAN_RELEASE_VERSION: ${{ github.event.release.tag_name }} |
There was a problem hiding this comment.
The environment variable COMAN_RELEASE_VERSION is set but may not be used elsewhere in the workflow.
#ai-review-inline
| - name: Checkout | ||
| uses: actions/checkout@v5 | ||
| - name: Strip version leading v | ||
| run: echo "COMAN_RELEASE_VERSION=${COMAN_RELEASE_VERSION##v}" >> $GITHUB_ENV |
There was a problem hiding this comment.
The command to strip 'v' prefix might fail if tag_name is empty or malformed.
#ai-review-inline
| // select cscs account | ||
| let source = config.value_source("cscs.account"); | ||
| if !source.1 | ||
| && !source.2 |
There was a problem hiding this comment.
Chained conditions with && can reduce readability; consider extracting into a helper function or variable.
#ai-review-inline
| if !source.1 && !source.2 { | ||
| let available_platforms: Vec<_> = get_available_compute_platforms() | ||
| .await | ||
| .unwrap_or(<ComputePlatform as VariantArray>::VARIANTS.to_vec()) |
There was a problem hiding this comment.
Using unwrap_or with a static list might hide errors; consider propagating the error instead.
#ai-review-inline
| let mut config = Config::new()?; | ||
|
|
||
| let source = config.value_source("cscs.current_platform"); | ||
| if !source.1 && !source.2 { |
There was a problem hiding this comment.
The condition '!source.1 && !source.2' may be unclear; consider using a more explicit check or adding a comment.
#ai-review-inline
| // Returns tuple of bool saying whether a values is set in (default, global, project local) config | ||
| pub fn value_source(&self, key_path: &str) -> Result<(bool, bool, bool)> { | ||
| Ok(( | ||
| pub fn value_source(&self, key_path: &str) -> (bool, bool, bool) { |
There was a problem hiding this comment.
Changed return type from Result to tuple, which could hide errors in calling code if not handled properly.
| pub fn value_source(&self, key_path: &str) -> (bool, bool, bool) { | |
| Consider keeping Result for better error propagation unless there's a specific reason to change. |
#ai-review-inline
| use lazy_static::lazy_static; | ||
| use serde::{Deserialize, Serialize}; | ||
| use strum_macros::{EnumIter, EnumString, VariantNames}; | ||
| use strum_macros::{EnumIter, EnumString, VariantArray, VariantNames}; |
There was a problem hiding this comment.
Added VariantArray derive macro which may increase memory usage for enums with many variants.
| use strum_macros::{EnumIter, EnumString, VariantArray, VariantNames}; | |
| Ensure that adding VariantArray is intentional and doesn't impact performance negatively. |
#ai-review-inline
| } | ||
|
|
||
| #[derive(Clone, Debug, Serialize, Deserialize, Default, strum::Display, EnumString, VariantNames, EnumIter)] | ||
| #[derive( |
There was a problem hiding this comment.
The enum definition now includes VariantArray, which requires careful consideration of memory and compile-time costs.
| #[derive( | |
| Verify that this addition aligns with performance requirements of the application. |
#ai-review-inline
| use vergen_gix::{BuildBuilder, CargoBuilder, Emitter, GixBuilder}; | ||
|
|
||
| fn main() -> Result<()> { | ||
| if let Ok(val) = std::env::var("COMAN_RELEASE_VERSION") { |
There was a problem hiding this comment.
Consider adding error handling for the environment variable parsing to avoid unexpected behavior if the variable is not a valid string.
| if let Ok(val) = std::env::var("COMAN_RELEASE_VERSION") { | |
| if let Ok(val) = std::env::var("COMAN_RELEASE_VERSION") { | |
| println!("cargo:rustc-env=CARGO_PKG_VERSION={}", val); | |
| } else { | |
| eprintln!("Warning: COMAN_RELEASE_VERSION not set or invalid"); | |
| } |
#ai-review-inline
| if let Ok(val) = std::env::var("COMAN_RELEASE_VERSION") { | ||
| println!("cargo:rustc-env=CARGO_PKG_VERSION={}", val); | ||
| } | ||
| println!("cargo:rerun-if-env-changed=COMAN_RELEASE_VERSION"); |
There was a problem hiding this comment.
The rerun-if-env-changed instruction should be placed before any conditional logic to ensure proper dependency tracking.
| println!("cargo:rerun-if-env-changed=COMAN_RELEASE_VERSION"); | |
| println!("cargo:rerun-if-env-changed=COMAN_RELEASE_VERSION"); | |
| if let Ok(val) = std::env::var("COMAN_RELEASE_VERSION") { | |
| println!("cargo:rustc-env=CARGO_PKG_VERSION={}", val); | |
| } |
#ai-review-inline
| tokio::spawn(async move { | ||
| match cscs_login(client_id, client_secret).await { | ||
| Ok(_) => event_tx.send(UserEvent::Cscs(CscsEvent::LoggedIn)).await.unwrap(), | ||
| Ok(_) => { |
There was a problem hiding this comment.
Consider extracting the config setup and platform logic into a separate function for better readability and testability.
| Ok(_) => { | |
| let mut config = match Config::new() { | |
| Ok(config) => config, | |
| Err(e) => { | |
| error_tx | |
| .send(format!( | |
| "{:#?}", | |
| Err::<(), Report>(e).wrap_err("Couldn't create config object") | |
| )) | |
| .await | |
| .unwrap(); | |
| return; | |
| } | |
| }; | |
| let source = config.value_source("cscs.current_platform"); | |
| if !source.1 && !source.2 { | |
| // don't override platform if it's already set | |
| if let Ok(available_platforms) = get_available_compute_platforms().await | |
| && !available_platforms.is_empty() | |
| && let Err(e) = config.set( | |
| "cscs.current_platform", | |
| available_platforms[0].to_string(), | |
| true, | |
| ) | |
| { | |
| error_tx | |
| .send(format!( | |
| "{:#?}", | |
| Err::<(), Report>(e).wrap_err("Couldn't set currnt platform") | |
| )) | |
| .await | |
| .unwrap(); | |
| return; | |
| } | |
| } | |
| event_tx.send(UserEvent::Cscs(CscsEvent::LoggedIn)).await.unwrap() |
#ai-review-inline
| if (api_client.list_systems().await).is_ok() { | ||
| config.set("cscs.current_platform", platform.to_string(), true)?; | ||
| break; | ||
| client_credentials_login(client_id_secret, client_secret_secret) |
There was a problem hiding this comment.
The function now returns unit type () instead of the token, which may break callers expecting the token.
| client_credentials_login(client_id_secret, client_secret_secret) | |
| Consider returning the token or updating callers to handle the new signature. |
#ai-review-inline
| } | ||
| } | ||
| Ok(platforms) | ||
| } |
There was a problem hiding this comment.
Returning Ok(()) here may hide important context about whether the token was successfully retrieved.
| } | |
| Return the token or a meaningful result indicating success/failure. |
#ai-review-inline
coman/src/cscs/handlers.rs
Outdated
| Ok(access_token) => { | ||
| let mut platforms = Vec::new(); | ||
| for platform in ComputePlatform::iter() { | ||
| let api_client = CscsApi::new(access_token.0.clone(), Some(platform.clone())).unwrap(); |
There was a problem hiding this comment.
Using unwrap() can cause a panic if CscsApi::new fails; consider handling the error more gracefully.
| let api_client = CscsApi::new(access_token.0.clone(), Some(platform.clone())).unwrap(); | |
| Replace `unwrap()` with proper error handling like `?` or a `match` expression. |
#ai-review-inline
Panaetius
force-pushed
the
automatic-release-version
branch
from
a789037 to
e41db2a
Compare
Panaetius
force-pushed
the
automatic-release-version
branch
from
e41db2a to
b76e73a
Compare
1 participant
No description provided.