Added the features selection to the CLI wizard #265
Conversation
Summary of ChangesHello @Hahihula, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the CLI wizard's functionality by integrating a feature selection mechanism for ESP-IDF installations. The primary goal is to provide users with greater control over which components of ESP-IDF are installed, thereby optimizing the setup process and allowing for more tailored development environments. This change introduces new core logic for handling feature metadata and refactors existing network-related utilities to support this new capability. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a feature selection wizard to the CLI, allowing users to choose which ESP-IDF features to install. The changes include adding new UI prompts, logic for fetching and parsing feature metadata from a URL, and refactoring URL generation logic. My review focuses on improving code maintainability by reducing duplication and ensuring the correct use of asynchronous functions in the async runtime.
| let requirements_files = match RequirementsMetadata::from_url(&req_url) { | ||
| Ok(files) => files, | ||
| Err(err) => { | ||
| warn!("{}: {}. {}", t!("wizard.requirements.read_failure"), err, t!("wizard.features.selection_unavailable")); | ||
| return Err(err.to_string()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The run_wizzard_run function is async, but it's using the blocking RequirementsMetadata::from_url to fetch the requirements file. This will block the async runtime. The idf_im_lib::idf_features module provides an async version, from_url_async, which should be used here with .await.
| let requirements_files = match RequirementsMetadata::from_url(&req_url) { | |
| Ok(files) => files, | |
| Err(err) => { | |
| warn!("{}: {}. {}", t!("wizard.requirements.read_failure"), err, t!("wizard.features.selection_unavailable")); | |
| return Err(err.to_string()); | |
| } | |
| }; | |
| let requirements_files = match RequirementsMetadata::from_url_async(&req_url).await { | |
| Ok(files) => files, | |
| Err(err) => { | |
| warn!("{}: {}. {}", t!("wizard.requirements.read_failure"), err, t!("wizard.features.selection_unavailable")); | |
| return Err(err.to_string()); | |
| } | |
| }; |
| // Interactive selection from filtered features | ||
| let items: Vec<String> = filtered_features | ||
| .iter() | ||
| .map(|f| { | ||
| format!( | ||
| "{} {} - {}", | ||
| if f.optional { "[ ]" } else { "[*]" }, | ||
| f.name, | ||
| f.description.as_deref().unwrap_or("No description") | ||
| ) | ||
| }) | ||
| .collect(); | ||
|
|
||
| let defaults: Vec<bool> = filtered_features.iter().map(|f| !f.optional).collect(); | ||
|
|
||
| let selections = MultiSelect::with_theme(&ColorfulTheme::default()) | ||
| .with_prompt("Select ESP-IDF features (Space to toggle, Enter to confirm)") | ||
| .items(&items) | ||
| .defaults(&defaults) | ||
| .interact() | ||
| .map_err(|e| format!("Selection failed: {}", e))?; | ||
|
|
||
| if selections.is_empty() { | ||
| return Err("No features selected".to_string()); | ||
| } | ||
|
|
||
| Ok(selections | ||
| .into_iter() | ||
| .map(|idx| filtered_features[idx].clone()) | ||
| .collect()) | ||
| } |
There was a problem hiding this comment.
The interactive selection logic in this function is very similar to the logic in select_features_interactive. To avoid code duplication and improve maintainability, consider extracting the common parts into a private helper function. This helper could handle building the MultiSelect prompt, interacting with the user, and returning the selected features.
For example, you could create a function like this:
fn prompt_user_for_features(
features: &[&FeatureInfo],
prompt_message: &str,
) -> Result<Vec<FeatureInfo>, String> {
// ... implementation of the MultiSelect dialog ...
}Both select_features_interactive and select_features_advanced could then call this helper with the appropriate list of features and prompt message.
| let repo_name = match repository { | ||
| Some(repo) => repo.to_string(), | ||
| None => { | ||
| if mirror.map_or(false, |m| m.contains("https://gitee.com/")) { | ||
| "EspressifSystems/esp-idf".to_string() | ||
| } else { | ||
| "espressif/esp-idf".to_string() | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
The logic to determine repo_name is duplicated from get_repo_url. To avoid code duplication and improve maintainability, this logic could be extracted into a private helper function, for example get_repo_name(repository: Option<&str>, mirror: Option<&str>) -> String. Both get_repo_url and get_raw_file_url could then use this helper function.
Hahihula
requested review from
Fabricio-ESP,
alirana01,
brianignacio5 and
kolipakakondal
|
We need to add the translations for the GUI on this new screen. On the CLI the selection squares and the parentheses makes it unclear what is the actual selection. Note that you can actually deselect the "core", should we allow installation without the core? |
we can hardly do that, as the texts come from this file: https://github.com/espressif/esp-idf/blob/master/tools/requirements.json |
should be better now: |
Hahihula
force-pushed
the
EIM-250-get-features-from-idf
branch
3 times, most recently
from
e0e1a7e to
83432ac
Compare
|
@Hahihula hi ! CLI version: install all necessary packages on all platforms ✅ But the Windows_GUI version does not install websocket-client: |
btw, the issue related to deselection of the "core" packages in CLI version still persist. 
|
Hahihula
force-pushed
the
EIM-250-get-features-from-idf
branch
5 times, most recently
from
8ce8e25 to
e921067
Compare
Added the features selection to the GUI wizard fixed store wizard total step count updated error handling windows gui installation now locks the mutex update the logic of reading config from the file and overriding it with cli args
Hahihula
force-pushed
the
EIM-250-get-features-from-idf
branch
from
e921067 to
ef96956
Compare
CLI selection of features:
and GUI: