IEP-1333: ESP-IDF Manager: Cannot invoke "String.indexOf(int)" because "value" is null #1058
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 actionlint
🔇 Additional comments (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java (1 hunks)
🔇 Additional comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java (1)
Line range hint
1-624: Overall assessment of changes to ToolsJob.javaThe changes to the
processExportCmdOutputmethod improve the handling of IDF version extraction by adding a fallback mechanism. This enhancement addresses a potential issue where the version might not be found in the command output.While this is a positive change, there are opportunities for further improvement in error handling, logging, and code style as detailed in the previous comment. These suggestions would make the code more robust and maintainable.
The rest of the file remains unchanged, and the overall structure and functionality of the
ToolsJobclass are preserved.
| else | ||
| { | ||
| idfToolSet.setIdfVersion(idfToolSet.getEnvVars().computeIfAbsent(IDFEnvironmentVariables.ESP_IDF_VERSION, k -> StringUtil.EMPTY)); | ||
| } |
There was a problem hiding this comment.
Improve error handling and code style for IDF version extraction
The addition of a fallback mechanism for retrieving the IDF version is a good improvement. However, there are a few areas that could be enhanced:
- The indentation of the
elseblock is incorrect. It should be aligned with theifstatement above. - Consider adding a log message when falling back to the environment variable, to help with debugging.
- Instead of using
StringUtil.EMPTYas the default value, it might be better to use a more descriptive default or throw an exception if the version can't be determined.
Here's a suggested improvement:
if (matcher.find())
{
idfToolSet.setIdfVersion(matcher.group(1));
}
- else
- {
- idfToolSet.setIdfVersion(idfToolSet.getEnvVars().computeIfAbsent(IDFEnvironmentVariables.ESP_IDF_VERSION, k -> StringUtil.EMPTY));
- }
+else
+{
+ String envVersion = idfToolSet.getEnvVars().get(IDFEnvironmentVariables.ESP_IDF_VERSION);
+ if (envVersion != null && !envVersion.isEmpty()) {
+ Logger.log("IDF version not found in command output. Using version from environment: " + envVersion);
+ idfToolSet.setIdfVersion(envVersion);
+ } else {
+ Logger.log("Unable to determine IDF version from command output or environment variables.");
+ throw new IllegalStateException("Unable to determine IDF version");
+ }
+}This change improves error handling, adds logging, and throws an exception if the version can't be determined, which is better than silently using an empty string.
Committable suggestion was skipped due to low confidence.
| else | ||
| { | ||
| idfToolSet.setIdfVersion(idfToolSet.getEnvVars().computeIfAbsent(IDFEnvironmentVariables.ESP_IDF_VERSION, k -> StringUtil.EMPTY)); | ||
| } |
There was a problem hiding this comment.
Not sure if this is the right way to approach the problem, I think the issue was because regular expression is not capturing the esp-idf version when it was on master branch
There was a problem hiding this comment.
Actually the output from the python is like this
Users/aliazamrana/.espressif/python_env/idf5.4_py3.9_env/lib/python3.9/site-packages/urllib3/__init__.py:35: NotOpenSSLWarning: urllib3 v2 only supports OpenSSL 1.1.1+, currently the 'ssl' module is compiled with 'LibreSSL 2.8.3'. See: https://github.com/urllib3/urllib3/issues/3020
warnings.warn(
ESP-IDF v5.4-dev-3201-g46acfdce96
which makes it difficult to have a single regex to parse all the possibilities so it would be better I think to fallback to the env variable that is being exported by the export script
There was a problem hiding this comment.
idf.py --version output on master
/Users/kondalkolipaka/.espressif/python_env/idf5.4_py3.9_env/lib/python3.9/site-packages/urllib3/__init__.py:35: NotOpenSSLWarning: urllib3 v2 only supports OpenSSL 1.1.1+, currently the 'ssl' module is compiled with 'LibreSSL 2.8.3'. See: https://github.com/urllib3/urllib3/issues/3020
warnings.warn(
ESP-IDF v5.4-dev-3201-g46acfdce96
on 5.2.2
ESP-IDF v5.2.2-dirty
|
@AndriiFilippov please verify |
|
@alirana01 hi ! Tested under: Windows 11 ✅ Could you please check if you see this message as well or this is my local issue. |
Description
Please see ticket
Fixes # (IEP-1333)
Type of change
Please delete options that are not relevant.
How has this been tested?
Try to reproduce using steps in ticket
Test Configuration:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores