IEP-974: Adding the IDE env versions to the esp_idf.json #781
Conversation
Previously only the path was changed in the file and now the esp_idf.json file is modified keeping the formatting proper and also adding the object for a different version
There was a problem hiding this comment.
Please see if this is fine here or we can somehow add it to main README.MD
There was a problem hiding this comment.
Suggestions needed on how we can improve this message
There was a problem hiding this comment.
Needed to use gson from google's api to make the file output pretty.
alirana01
changed the title
There was a problem hiding this comment.
Hi @alirana01. Added suggestions to improve the message. Code LGTM, I couldn't find visually any bugs, however, I'm finding methods in this class are a little bit complex, and maybe we want to refactor them in the future.
My ideas on what we can add in the future:
- We could move all json related operations to separate classes and cover them with unit tests.
- I'm finding earlyStartup() method has at least two reasons to change (violation SRP), so mb we want to create another class that implements IStartup.
Let me know what you think about it. Do you have any other ideas on how to make it more maintainable and robust?
| java.util.regex.Pattern p = java.util.regex.Pattern.compile("([0-9][.][0-9])"); //$NON-NLS-1$ | ||
| java.util.regex.Matcher m = p.matcher(version); |
There was a problem hiding this comment.
Why not just use Pattern and Matcher and import java.util.regex.Matcher; import java.util.regex.Pattern; here?
| ToolsInitializationDifferentPathMessageBoxTitle=Different IDF path found in the config file | ||
| ToolsInitializationDifferentPathMessageBoxMessage=A different IDF path was found in the startup configuration file do you want to install the tools in this path or keep the older path\nNew Path: {0}\nOld Path: {1} | ||
| ToolsInitializationDifferentPathMessageBoxTitle=Different IDF path found in IDE Environment | ||
| ToolsInitializationDifferentPathMessageBoxMessage=Different IDF tools are installed in the IDE Environment: {0} than the one selected in startup config file for IDE (esp_idf.json).\nIf you want to keep the IDF tools in the IDE Environment select Yes(This will also add the tools to esp_idf.json).\nIf you want to revert to the last used tools in esp_idf.json press No. |
There was a problem hiding this comment.
what about rephrasing it like this:
- 1 version:
The IDE Environment has different IDF tools installed compared to the ones selected in the startup-config file for the IDE (esp_idf.json).
To keep the IDF tools {0} in the IDE Environment and update the tools in esp_idf.json, select "Yes."
To revert to the last used tools in esp_idf.json, press "No."
However, I found version 1 too verbose
- 2 version:
The IDF tools installed in the IDE Environment are different from the ones selected in the startup-config file (esp_idf.json) for the IDE. Do you want to keep the IDF tools {0} in the IDE Environment and update the tools in esp_idf.json?
what do you think?
There was a problem hiding this comment.
The second version is misleading to my understanding as I think it can confuse the user like they can press No thinking that the tools wont be added to json file but instead we are trying to revert the tools back to the original installation. This would be okay if we didnt have the usecase where the tools are updated via the IDE offline installer.
| JSONObject selectedIDFInfo = (JSONObject) list.get(idfVersionId); | ||
| String idfPath = (String) selectedIDFInfo.get(IDF_PATH); | ||
| IDFEnvironmentVariables idfEnvMgr = new IDFEnvironmentVariables(); | ||
| return !idfEnvMgr.getEnvValue(IDFEnvironmentVariables.IDF_PATH).equals(idfPath); |
There was a problem hiding this comment.
idfEnvMgr.getEnvValue(IDFEnvironmentVariables.IDF_PATH) will return null when IDE is not configured with the IDF_PATH(IDE is not launched and configured not even once). That leads to NPE
There was a problem hiding this comment.
In case of multiple back slashes in the path, this might fail.
IDE would have configured IDF_PATH to C:\esp\esp-idf-5.0 and windows installer would have set to C:\\esp\\esp-idf-5.0 as per equals() both will be different
There was a problem hiding this comment.
good catch. Probably we could use Paths.get().normalize() here
There was a problem hiding this comment.
good catch. Probably we could use Paths.get().normalize() here
I dont think using normalize is a good idea simply comparison like using Paths.get(path).toAbsolutePath().compareTo(pathToCompare) == 0
should be enough for us considering that we null check first.
| String version = IDFUtil.getEspIdfVersion(); | ||
| java.util.regex.Pattern p = java.util.regex.Pattern.compile("([0-9][.][0-9])"); //$NON-NLS-1$ | ||
| java.util.regex.Matcher m = p.matcher(version); | ||
| if (m.find()) | ||
| { | ||
| version = m.group(0); | ||
| } | ||
| updateEspIdfJsonFile(idf_json_file, idfEnvMgr.getEnvValue(IDFEnvironmentVariables.IDF_PATH), | ||
| IDFUtil.getIDFPythonEnvPath(), | ||
| version); |
There was a problem hiding this comment.
What if the user updated esp-idf(let's say 5.0 to 5.1) through windows installer, in this case isVersionDifferentInFile is true considering we compare esp-idf path from IDE and esp-idf.json
Don't we export the paths in this case?
Description
Previously only the path was changed in the file and now the esp_idf.json file is modified keeping the formatting proper and also adding the object for a different version.
Also improved the message to user
Fixes # (IEP-974)
Type of change
Please delete options that are not relevant.
How has this been tested?
Use the testing from IEP-952 (PR)
Additionally verify that the updated version in the IDE is also added to the esp_idf.json file and is the activated one
Test Configuration:
Checklist