IEP-1018 Automation of hints viewer#809
Conversation
sigmaaa
left a comment
sigmaaa
left a comment
There was a problem hiding this comment.
Self reviewed. Need to add documentation and test coverage for EspIdfErrorParser
sigmaaa
changed the title
|
@sigmaaa hi ! ESP-IDF: v5.0 - v5.1 release 5.1 / 5.1.1 LGTM 👍 able to enable / disable hint feature. able to see useful hints. release 5.0 / 5.0.3 when hint option is selected unable to build a project.
LOGS: |
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/BuildView.java
Outdated
Show resolved
Hide resolved
WalkthroughThis pull request introduces automated build hints to the ESP-IDF Eclipse plugin. It adds a new view for displaying build hints, modifies the preferences page to enable/disable this feature, and includes changes to handle regular expressions more safely. The PR also includes relevant tests and UI updates. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 14
Files selected for processing (15)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/EspIdfErrorParser.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (3 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/resources/PopupDialog.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ParitionSizeHandler.java (3 hunks)
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties (1 hunks)
- bundles/com.espressif.idf.ui/plugin.xml (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (6 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/BuildView.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (5 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties (1 hunks)
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/build/test/EspIdfErrorParserTest.java (1 hunks)
Files skipped from review due to trivial changes (8)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/resources/PopupDialog.java
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties
- bundles/com.espressif.idf.ui/plugin.xml
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties
Additional comments (Suppressed): 13
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ParitionSizeHandler.java (3)
57-61: The
getPartitionTable()method now returns the output of the process as a string directly. This change seems to be fine, but ensure that all calls to this function throughout the codebase have been updated to handle the new return type.65-69: The
startProcess()method has been refactored to return the started process directly instead of storing it in a local variable first. This is a good improvement as it simplifies the code and reduces unnecessary variable usage.99-105: The
firePropertyChange()method call in thecheckRemainingSize()method has been updated to usePopupDialog.LOW_PARTITION_SIZE.name()as the property name instead ofnull. This is a good change as it makes the code more readable and understandable by clearly indicating what property change event is being fired.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (5)
35-35: The new button
automateHintsBtnhas been added to the preference page. Ensure that this button is correctly initialized and its state is properly managed.75-87: A new group for build settings has been added to the main composite of the preference page. This group contains a checkbox for enabling/disabling automated build hints. The checkbox's selection status is retrieved from the preference store. This looks good and follows the existing pattern for adding controls to the preference page.
151-156: The state of the
automateHintsBtnis saved in the preference store when the user clicks the OK button. This is consistent with how other preferences are saved.171-175: The
performDefaults()method has been updated to reset theautomateHintsBtnto its default state. This is consistent with how other preferences are reset to their default values.182-186: The
initializeDefaults()method has been updated to set the default value for theAUTOMATE_BUILD_HINTS_STATUSpreference. This is consistent with how other preferences are initialized.bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (2)
105-111: The new import
import com.espressif.idf.core.util.HintsUtil;has been added. Ensure that this class is available in the project and it's correctly implemented.509-515: The shutdown method is now called on all console parsers after the process finishes. This is a good practice as it ensures that any resources held by the parsers are released.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (1)
- 72-88: The switch statement inside the lambda function for handling property change events has been refactored to handle two cases:
LOW_PARTITION_SIZEandAVAILABLE_HINTS. Ensure that all places where these events are fired have been updated accordingly.tests/com.espressif.idf.core.test/src/com/espressif/idf/core/build/test/EspIdfErrorParserTest.java (1)
- 47-59: The test
process_line_returns_false_if_no_hint_found_for_error_lineis well written and covers the scenario where no hint is found for an error line. It's good to see that you're testing both positive and negative scenarios.bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/EspIdfErrorParser.java (1)
- 64-69: Ensure that the
shutdownmethod is called at the end of each parsing session to clear theallMatchesList. If it's not called, the list will keep growing with entries from previous sessions, which could lead to incorrect results and memory issues.
| private void createColumns(Composite container) | ||
| { | ||
| TableViewerColumn col = createTableViewerColumn(titles[0]); | ||
| col.setLabelProvider(new ColumnLabelProvider() | ||
| { | ||
| @Override | ||
| public Image getImage(Object element) | ||
| { | ||
| return PlatformUI.getWorkbench().getSharedImages().getImage(ISharedImages.IMG_OBJS_ERROR_TSK); | ||
| } | ||
|
|
||
| @Override | ||
| public String getText(Object element) | ||
| { | ||
| return ((ReHintPair) element).getRe(); | ||
| } | ||
| }); | ||
| col = createTableViewerColumn(titles[1]); | ||
| col.setLabelProvider(new ColumnLabelProvider() | ||
| { | ||
| @Override | ||
| public Image getImage(Object element) | ||
| { | ||
| return PlatformUI.getWorkbench().getSharedImages().getImage(ISharedImages.IMG_OBJS_INFO_TSK); | ||
| } | ||
|
|
||
| @Override | ||
| public String getText(Object element) | ||
| { | ||
| return ((ReHintPair) element).getHint(); | ||
| } | ||
| }); | ||
| col.getViewer().addDoubleClickListener(event -> | ||
| { | ||
| StructuredSelection selection = (StructuredSelection) event.getViewer().getSelection(); | ||
| MessageDialog.openInformation(container.getShell(), Messages.HintDetailsTitle, | ||
| ((ReHintPair) selection.getFirstElement()).getHint()); | ||
|
|
||
| }); | ||
| } |
There was a problem hiding this comment.
The createColumns method is creating two columns with similar properties. This code could be refactored to avoid repetition. Consider creating a method that takes the title, image, and text as parameters and creates a column.
| console.getErrorStream().write(String.format(Messages.CMakeBuildConfiguration_Failure, "")); //$NON-NLS-1$ | ||
| throw new CmakeBuildException(); | ||
| } | ||
|
|
||
| watchProcess(p, new IConsoleParser[] { epm, new StatusParser() }); | ||
| boolean buildHintsStatus = Platform.getPreferencesService().getBoolean(IDFCorePlugin.PLUGIN_ID, | ||
| IDFCorePreferenceConstants.AUTOMATE_BUILD_HINTS_STATUS, | ||
| IDFCorePreferenceConstants.AUTOMATE_BUILD_HINTS_DEFAULT_STATUS, null); | ||
| IConsoleParser[] consoleParsers = buildHintsStatus | ||
| ? new IConsoleParser[] { epm, new StatusParser(), | ||
| new EspIdfErrorParser(HintsUtil.getReHintsList(new File(HintsUtil.getHintsYmlPath()))) } | ||
| : new IConsoleParser[] { epm, new StatusParser() }; | ||
| watchProcess(p, consoleParsers); | ||
|
|
||
| final String isSkip = System.getProperty("skip.idf.components"); //$NON-NLS-1$ | ||
| if (!Boolean.parseBoolean(isSkip)) |
There was a problem hiding this comment.
The code now checks for a preference setting to decide whether to use the EspIdfErrorParser or not. This is a good approach as it allows users to toggle the feature on/off. However, there seems to be no error handling if HintsUtil.getReHintsList(new File(HintsUtil.getHintsYmlPath())) fails (e.g., file not found, parsing error). Consider adding appropriate error handling here.
- new EspIdfErrorParser(HintsUtil.getReHintsList(new File(HintsUtil.getHintsYmlPath())))
+ List<Hint> hints = Collections.emptyList();
+ try {
+ hints = HintsUtil.getReHintsList(new File(HintsUtil.getHintsYmlPath()));
+ } catch (Exception e) {
+ Logger.log(e);
+ }
+ new EspIdfErrorParser(hints)| private static final String BUILDHINTS_ID = "com.espressif.idf.ui.views.buildhints"; //$NON-NLS-1$ | ||
|
|
There was a problem hiding this comment.
The constant BUILDHINTS_ID is defined but there are no comments explaining its purpose. It would be beneficial to add a comment describing what this constant represents in the context of your application.
| catch (PartInitException e) | ||
| { |
There was a problem hiding this comment.
Exception logging is good, but it might be more helpful to provide some context or additional information about what operation was being attempted when the exception occurred. This can make debugging easier if an error occurs.
| @SuppressWarnings("unchecked") | ||
| private void openAvailableHintsDialog(PropertyChangeEvent evt) | ||
| { | ||
| Display.getDefault().asyncExec(() -> | ||
| { | ||
| List<ReHintPair> erroHintPairs = (List<ReHintPair>) evt.getNewValue(); | ||
| // if list is empty we don't want to change focus from the console output | ||
| if (erroHintPairs.isEmpty()) | ||
| { | ||
| updateValuesInBuildView(erroHintPairs); | ||
| return; | ||
| } | ||
| try | ||
| { | ||
| PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage() | ||
| .showView(BUILDHINTS_ID); | ||
| } | ||
| catch (PartInitException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| updateValuesInBuildView(erroHintPairs); | ||
| } | ||
| ); | ||
|
|
||
| } |
There was a problem hiding this comment.
This method opens a dialog with available hints. However, it seems like it's doing more than just opening a dialog. It's also updating the values in the BuildView. Consider renaming this method to better reflect its functionality or splitting it into two methods: one for opening the dialog and another for updating the BuildView.
| private void updateValuesInBuildView(List<ReHintPair> erroHintPairs) | ||
| { | ||
| BuildView view = ((BuildView) PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage() | ||
| .findView(BUILDHINTS_ID)); | ||
| if (view != null) | ||
| { | ||
| view.updateReHintsPairs(erroHintPairs); | ||
| } | ||
| } |
There was a problem hiding this comment.
This method updates the values in the BuildView. However, it doesn't check whether the erroHintPairs parameter is null before using it. This could lead to a NullPointerException if the method is called with a null argument. Consider adding a null check at the beginning of this method.
+ if (erroHintPairs == null) {
+ throw new IllegalArgumentException("erroHintPairs cannot be null");
+ }| void process_line_returns_true_if_hint_available_for_error_line() | ||
| { | ||
| List<ReHintPair> reHintPairs = new ArrayList<>(); | ||
| reHintPairs.add( | ||
| new ReHintPair(ERROR_REGEX, StringUtil.EMPTY)); | ||
| String errorLine = ERROR_LINE; | ||
| EspIdfErrorParser ep = new EspIdfErrorParser(reHintPairs); | ||
|
|
||
| boolean actualResult = ep.processLine(errorLine); | ||
|
|
||
| assertTrue(actualResult); | ||
| } |
There was a problem hiding this comment.
The test process_line_returns_true_if_hint_available_for_error_line is well written and covers the scenario where a hint is available for an error line. However, it would be beneficial to also verify that the correct hint was returned. This can be done by adding an assertion to check the hint value.
| @Test | ||
| void shutdown_should_trigger_property_change_listener_with_error_hints_pairs_as_new_value() | ||
| { | ||
| List<ReHintPair> actualReHintPair = new ArrayList<>(); | ||
| OpenDialogListenerSupport.getSupport().addPropertyChangeListener(evt -> { | ||
| PopupDialog popupDialog = PopupDialog.valueOf(evt.getPropertyName()); | ||
| if (popupDialog.equals(PopupDialog.AVAILABLE_HINTS)) | ||
| actualReHintPair.addAll((List<ReHintPair>) evt.getNewValue()); | ||
| }); | ||
| List<ReHintPair> reHintPairs = new ArrayList<>(); | ||
| String expectedHint = EXPECTED_HINT; | ||
| reHintPairs.add( | ||
| new ReHintPair(ERROR_REGEX, expectedHint)); | ||
| String errorLine = ERROR_LINE; | ||
|
|
||
| EspIdfErrorParser ep = new EspIdfErrorParser(reHintPairs); | ||
| boolean actualResult = ep.processLine(errorLine); | ||
| ep.shutdown(); | ||
|
|
||
| assertTrue(actualResult); | ||
| assertEquals(expectedHint, actualReHintPair.get(0).getHint()); | ||
| assertEquals(errorLine, actualReHintPair.get(0).getRe()); | ||
| } |
There was a problem hiding this comment.
The test shutdown_should_trigger_property_change_listener_with_error_hints_pairs_as_new_value is well written and covers the scenario where the shutdown method should trigger a property change listener with error hints pairs as new value. However, it would be beneficial to also verify that the property change listener is not triggered when there are no error hints pairs. This can be done by adding another test case.
| public EspIdfErrorParser(List<ReHintPair> reHintPairs) | ||
| { | ||
| this.reHintsList = reHintPairs; | ||
| this.allMatchesList = new ArrayList<>(); | ||
| } |
There was a problem hiding this comment.
The constructor EspIdfErrorParser(List<ReHintPair> reHintPairs) does not check if the passed list reHintPairs is null. This can lead to a NullPointerException in the processLine method when it tries to iterate over reHintsList. It's a good practice to validate constructor parameters to avoid potential runtime exceptions.
- public EspIdfErrorParser(List<ReHintPair> reHintPairs)
- {
- this.reHintsList = reHintPairs;
- this.allMatchesList = new ArrayList<>();
- }
+ public EspIdfErrorParser(List<ReHintPair> reHintPairs)
+ {
+ if (reHintPairs == null) {
+ throw new IllegalArgumentException("reHintPairs cannot be null");
+ }
+ this.reHintsList = reHintPairs;
+ this.allMatchesList = new ArrayList<>();
+ }| for (ReHintPair reHintEntry : reHintsList) | ||
| { | ||
| boolean isRegexMatchesWithProcessedString = Pattern.compile(reHintEntry.getRe()).matcher(paramString) | ||
| .find(); | ||
| if (isRegexMatchesWithProcessedString) | ||
| { | ||
| allMatchesList.add(new ReHintPair(paramString, reHintEntry.getHint())); | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The regular expression pattern is compiled for every line processed, which can be inefficient if the number of lines or patterns is large. Consider compiling the patterns once during initialization and reusing them.
- public boolean processLine(String paramString)
- {
- for (ReHintPair reHintEntry : reHintsList)
- {
- boolean isRegexMatchesWithProcessedString = Pattern.compile(reHintEntry.getRe()).matcher(paramString)
- .find();
- if (isRegexMatchesWithProcessedString)
- {
- allMatchesList.add(new ReHintPair(paramString, reHintEntry.getHint()));
- return true;
- }
- }
- return false;
- }
+ public boolean processLine(String paramString)
+ {
+ for (ReHintPair reHintEntry : reHintsList)
+ {
+ boolean isRegexMatchesWithProcessedString = reHintEntry.getPattern().matcher(paramString).find();
+ if (isRegexMatchesWithProcessedString)
+ {
+ allMatchesList.add(new ReHintPair(paramString, reHintEntry.getHint()));
+ return true;
+ }
+ }
+ return false;
+ }In the above diff, I'm assuming that you will modify the ReHintPair class to store a Pattern object instead of a String for the regex. You would compile the pattern when creating the ReHintPair instance.
WalkthroughThis change introduces automated build hints to the ESP-IDF Eclipse plugin. It adds a new preference for enabling/disabling this feature, parses console output for errors and matches them with hints, displays these hints in a new view, and provides tests for the error parser. Changes
TipsChat with CodeRabbit Bot (
|
|
Hi @sigmaaa ! Tested under: Issue still persist. |
|
Hi @AndriiFilippov, @kolipakakondal, fixed in the last commit by catching incompatible Python regex. This regex is fixed on a version higher than 5.0.3 on esp-idf side as well, that's why I was not able to reproduce this problem earlier |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Files selected for processing (15)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/EspIdfErrorParser.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (3 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/resources/PopupDialog.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ParitionSizeHandler.java (3 hunks)
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties (1 hunks)
- bundles/com.espressif.idf.ui/plugin.xml (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (6 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/BuildView.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (5 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties (1 hunks)
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/build/test/EspIdfErrorParserTest.java (1 hunks)
Files skipped from review due to trivial changes (7)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties
- bundles/com.espressif.idf.ui/plugin.xml
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/build/test/EspIdfErrorParserTest.java
Additional comments (Suppressed): 20
bundles/com.espressif.idf.core/src/com/espressif/idf/core/resources/PopupDialog.java (1)
- 1-6: The new
PopupDialogenum is introduced to manage the types of popup dialogs in the application. Ensure that all references to these dialog types throughout the codebase have been updated to use this enum.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (5)
35-35: The new button
automateHintsBtnis added to the class. Ensure that it is properly disposed of in the appropriate lifecycle method to prevent memory leaks.67-87: A new section for build settings is added to the main composite. The code looks good and follows the same pattern as other sections. The checkbox for automated build hints is created, its text and tooltip are set, and its initial state is retrieved from the preference store.
151-156: The state of the
automateHintsBtnis saved to the preference store when the user clicks the OK button. This is consistent with how other preferences are saved.171-175: The state of the
automateHintsBtnis restored from the preference store when the defaults are loaded. This is consistent with how other preferences are restored.182-186: The default state of the
automateHintsBtnis set in theinitializeDefaults()method. This is consistent with how other default preferences are set.bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ParitionSizeHandler.java (2)
63-69: The
startProcess()method has been refactored to remove the creation of theProcessobject from theProcessBuilder. This change simplifies the code and makes it easier to understand. However, please ensure that this change does not affect other parts of the code that may rely on the previous behavior.99-105: The property change event fired in the
checkRemainingSize()method now uses a constant (PopupDialog.LOW_PARTITION_SIZE.name()) instead ofnullfor the property name. This is a good practice as it avoids potential null pointer exceptions and makes the code more readable. Ensure that the listener for this event is updated to handle this new property name.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (8)
16-17: The import of
org.eclipse.cdt.cmake.core.internal.Activatorseems to be unused in the new hunks. Please verify if this is necessary or can be removed.35-48: The imports for
ReHintPair,BuildView, andPopupDialogare new additions. Ensure that these classes are correctly implemented and used within the code.54-54: The constant
BUILDHINTS_IDis a new addition. Make sure it's being used appropriately in the code.75-88: The property change listener has been updated to handle different types of popup dialogs. This is a good improvement as it makes the code more flexible and easier to extend with new dialog types in the future.
219-220: Ensure proper error handling is in place for exceptions caught here. It might be beneficial to add some user-facing error messages or logs for debugging purposes.
202-227: This method opens a dialog with available hints. The logic seems fine, but ensure that the
BUILDHINTS_IDview exists and can be shown without issues.229-237: This method updates the values in the BuildView. Ensure that the
updateReHintsPairsmethod in theBuildViewclass handles the list ofReHintPairobjects correctly.239-246: This method opens a dialog warning about low partition size. The implementation looks correct.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (2)
105-109: The new import
import com.espressif.idf.core.util.HintsUtil;is added to use theHintsUtilclass. Ensure that this class exists and is accessible from this package.512-512: The shutdown method is now called on all console parsers after the build process completes. This is a good practice as it ensures any resources held by the parsers are released promptly.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java (2)
8-8: The new string
EspresssifPreferencesPage_BuildGroupTxtis added. Ensure that this string is properly initialized and used in the appropriate context.12-13: Two new strings
EspresssifPreferencesPage_SearchHintsCheckBtnandEspresssifPreferencesPage_SearchHintsTooltipare introduced for the checkbox to enable or disable automated build hints. Make sure these strings are correctly initialized and used where the checkbox is defined.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/EspIdfErrorParser.java
Show resolved
Hide resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ParitionSizeHandler.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/BuildView.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/BuildView.java
Show resolved
Hide resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 13
Files selected for processing (6)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/EspIdfErrorParser.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/ReHintPair.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/BuildView.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/HintsView.java (5 hunks)
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/build/test/EspIdfErrorParserTest.java (1 hunks)
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/HintsUtilTest.java (2 hunks)
Files skipped from review due to trivial changes (1)
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/HintsUtilTest.java
Additional comments (Suppressed): 4
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/ReHintPair.java (1)
- 35-38: The method
getRe()now returns anOptional<Pattern>, which is a good practice for handling null values. However, ensure that all calls to this method throughout the codebase have been updated to handle the returned Optional.tests/com.espressif.idf.core.test/src/com/espressif/idf/core/build/test/EspIdfErrorParserTest.java (1)
- 47-59: The test
process_line_returns_false_if_no_hint_found_for_error_linecorrectly checks the case where no hint is found for an error line. Good job on covering this edge case.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/HintsView.java (2)
11-24: The import statements are well organized and necessary for the functionality of the class. The removal of unused imports improves the readability and maintainability of the code.
33-38: The addition of
import com.espressif.idf.core.util.StringUtil;is a good practice as it provides utility methods for string manipulation which can be used throughout the class. The rest of the changes in this hunk do not alter the functionality of the code.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/ReHintPair.java
Show resolved
Hide resolved
...com.espressif.idf.core.test/src/com/espressif/idf/core/build/test/EspIdfErrorParserTest.java
Show resolved
Hide resolved
...com.espressif.idf.core.test/src/com/espressif/idf/core/build/test/EspIdfErrorParserTest.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/BuildView.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/BuildView.java
Show resolved
Hide resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/EspIdfErrorParser.java
Show resolved
Hide resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/EspIdfErrorParser.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/HintsView.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/HintsView.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/HintsView.java
Show resolved
Hide resolved
|
Hi @kolipakakondal, @alirana01, @AndriiFilippov |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/ReHintPair.java (1 hunks)
Additional comments (Suppressed): 1
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/ReHintPair.java (1)
- 36-39: The
getRe()method now returns anOptional<Pattern>, which is a good practice for handling potential null values. Ensure that all calls to this method throughout the codebase have been updated to handle the returnedOptional.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/ReHintPair.java
Show resolved
Hide resolved
|
@sigmaaa hi ! Tested under: ESP-IDF: v5.0 - v5.1 release 5.1 / 5.1.1 / release 5.0 / 5.0.3 - able to enable / disable hint feature. able to see useful hints. LGTM 👍 |
3 participants
Description
Added automated build hints viewer which appears after the build if there are available hints for founded errors. Also, can be turned on manually via Windows -> Show View -> Other... -> Espressif -> Build Hints
Fixes # (IEP-1018)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test 1:
#import spiram.hTest 2:
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
ReHintPairclass by usingPatternobjects instead ofString.EspIdfErrorParserclass and updated existing tests to accommodate changes in theReHintPairclass.