IEP-1109 Target provided based on target's id and not IDF target property #850

sigmaaa · 2023-11-03T12:55:25Z

Description

To reproduce this issue: edit the launch target. For example for the esp32 target select the esp32s2 target and edit the esp32s2 target for esp32. After that edit the launch or debug configuration and change the target to esp32s2 or esp32 -> after finishing the dialog you will receive notification about incorrect target for jtag flashing

Fixes # (IEP-1109)

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

How has this been tested?

Test 1:

verified steps that mentioned in description

Test Configuration:

ESP-IDF Version:
OS (Windows,Linux and macOS):

Dependent components impacted by this PR:

launch configuration target
debug configuration target

Checklist

Summary by CodeRabbit

Refactor
- Improved code readability and maintainability by introducing a new constant for attribute names and replacing hardcoded attribute names with this constant in various places.
Chores
- Updated the run method to filter launch targets based on attribute value, enhancing the code's efficiency.

coderabbitai · 2023-11-03T12:55:34Z

Walkthrough

The changes introduced in the codebase primarily focus on improving code readability and maintainability. A new constant LAUNCH_TARGET_NAME_ATTR has been introduced and used across multiple methods and files, replacing hardcoded attribute names and values, thus enhancing the code's consistency and ease of future modifications.

Changes

File Path	Summary
.../idf/debug/gdbjtag/openocd/ui/TabDebugger.java	Introduced a new constant `LAUNCH_TARGET_NAME_ATTR`. Updated `run` and `findSuitableTargetForSelectedItem` methods to use this constant, improving code readability and maintainability.
.../idf/launch/serial/ui/internal/CMakeMainTab2.java	Added a new constant `LAUNCH_TARGET_NAME_ATTR`. Replaced hardcoded attribute string in `findSuitableTargetForSelectedItem` and `run` methods with the new constant, enhancing code consistency.

Poem

🍂 As autumn leaves fall, so does old code retire, 🍁

Replaced by constants, to which we aspire. 🌟

No more hardcoding, in our code's domain, 🚫

With LAUNCH_TARGET_NAME_ATTR, we break the chain. 🔗

So here's to the coders, in their digital den, 🐇💻

Making changes, again and again. 🔄

As the season changes, so does our code, 🔄🍂

On the path of improvement, we continue to trod. 🛣️👣

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai help to get help.
@coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 05ea8a4 and f4bd3dd.

Files selected for processing (2)

bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java (3 hunks)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java (3 hunks)

Additional comments: 6

bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java (3)

103-103: The introduction of the constant LAUNCH_TARGET_NAME_ATTR is a good practice as it improves code readability and maintainability. It also reduces the risk of typos and inconsistencies.

278-280: The use of the constant LAUNCH_TARGET_NAME_ATTR in the filter method is a good practice. It ensures that the attribute name is consistent throughout the code.

553-555: The use of the constant LAUNCH_TARGET_NAME_ATTR to retrieve the attribute value is a good practice. It ensures that the attribute name is consistent throughout the code.

bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java (3)

88-88: The new constant LAUNCH_TARGET_NAME_ATTR is a good addition. It replaces the hardcoded attribute name, improving code readability and maintainability.

746-749: The getAttribute method is now using the new constant LAUNCH_TARGET_NAME_ATTR. This is a good change as it makes the code more maintainable and less prone to errors.

873-876: The findFirst method now uses a filter based on the attribute value using the constant LAUNCH_TARGET_NAME_ATTR. This is a good change as it makes the code more readable and maintainable.

AndriiFilippov · 2023-11-06T12:24:10Z

@sigmaaa hi !

Tested:
OS - Windows 10
ESP-IDF: v5.1.1

LGTM 👍

sigmaaa · 2023-11-06T13:10:02Z

...if.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java


 	// ------------------------------------------------------------------------

+	private static final String LAUNCH_TARGET_NAME_ATTR = "com.espressif.idf.launch.serial.core.idfTarget"; //$NON-NLS-1$


had to write this attribute like this. Maybe we can use a single class in the core package to store all attributes there

You can use IDFLaunchConstants.ATTR_IDF_TARGET

sigmaaa

self reviewed

kolipakakondal

LGTM

kolipakakondal · 2023-11-15T06:29:15Z

...if.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java


 	// ------------------------------------------------------------------------

+	private static final String LAUNCH_TARGET_NAME_ATTR = "com.espressif.idf.launch.serial.core.idfTarget"; //$NON-NLS-1$


You can use IDFLaunchConstants.ATTR_IDF_TARGET

fix: comparing launch targets based on idfTatget attr

f4bd3dd

sigmaaa requested review from AndriiFilippov, alirana01 and kolipakakondal November 3, 2023 12:55

sigmaaa self-assigned this Nov 3, 2023

coderabbitai bot reviewed Nov 3, 2023

View reviewed changes

AndriiFilippov approved these changes Nov 6, 2023

View reviewed changes

sigmaaa commented Nov 6, 2023

View reviewed changes

kolipakakondal approved these changes Nov 15, 2023

View reviewed changes

kolipakakondal added this to the v2.12.0 milestone Nov 15, 2023

kolipakakondal merged commit 5669571 into master Nov 15, 2023

kolipakakondal deleted the IEP-1109 branch November 15, 2023 06:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

IEP-1109 Target provided based on target's id and not IDF target property #850

IEP-1109 Target provided based on target's id and not IDF target property #850

Uh oh!

sigmaaa commented Nov 3, 2023 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 3, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Uh oh!

coderabbitai bot left a comment

Uh oh!

AndriiFilippov commented Nov 6, 2023

Uh oh!

sigmaaa Nov 6, 2023 •

edited

Loading

Uh oh!

kolipakakondal Nov 15, 2023

Uh oh!

sigmaaa left a comment

Uh oh!

kolipakakondal left a comment

Uh oh!

kolipakakondal Nov 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		// ------------------------------------------------------------------------

		private static final String LAUNCH_TARGET_NAME_ATTR = "com.espressif.idf.launch.serial.core.idfTarget"; //$NON-NLS-1$

IEP-1109 Target provided based on target's id and not IDF target property #850

IEP-1109 Target provided based on target's id and not IDF target property #850

Uh oh!

Conversation

sigmaaa commented Nov 3, 2023 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How has this been tested?

Dependent components impacted by this PR:

Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Poem

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

AndriiFilippov commented Nov 6, 2023

Uh oh!

sigmaaa Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolipakakondal Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

sigmaaa left a comment

Choose a reason for hiding this comment

Uh oh!

kolipakakondal left a comment

Choose a reason for hiding this comment

Uh oh!

kolipakakondal Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sigmaaa commented Nov 3, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 3, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)

sigmaaa Nov 6, 2023 •

edited

Loading