Obey inline component editor connection renames#1676
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the configuration-step connection-renaming logic to respect inline component editor “connection type” changes, including the ability to re-evaluate rename_connection renames in-place (without rebuilding the full parameter set). It also renames the underlying rename-calculation helper to a public calculate_connection_rename_operations method and updates tests accordingly.
Changes:
- Rename
_calculate_connection_rename_operations→calculate_connection_rename_operationsand update call sites/tests. - Track and re-apply/undo connection renames in
ParameterEditor.refresh_current_step_computed_parameters()when component data changes. - Add a GNSS-specific special case to force a MAVLink protocol value when applicable.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/test_data_model_configuration_step.py | Updates unit tests to use the new public calculate_connection_rename_operations name. |
| ardupilot_methodic_configurator/data_model_parameter_editor.py | Adds connection-rename tracking and in-place refresh logic when inline component edits change the evaluated connection type. |
| ardupilot_methodic_configurator/data_model_configuration_step.py | Renames the static helper to calculate_connection_rename_operations and updates internal usage. |
Comment on lines
+2369
to
+2376
| # Target changed — move the parameter to the new name | ||
| if current_target in self.current_step_parameters: | ||
| self._added_parameters.discard(current_target) | ||
| del self.current_step_parameters[current_target] | ||
| if orig_name in original_params: | ||
| self.current_step_parameters[new_target] = self._config_step_processor.create_ardupilot_parameter( | ||
| new_target, original_params[orig_name], self.current_file, self.fc_parameters | ||
| ) |
Comment on lines
+2358
to
+2367
| # Rename no longer applicable — restore original name | ||
| if current_target in self.current_step_parameters: | ||
| self._added_parameters.discard(current_target) | ||
| del self.current_step_parameters[current_target] | ||
| if orig_name in original_params: | ||
| self.current_step_parameters[orig_name] = self._config_step_processor.create_ardupilot_parameter( | ||
| orig_name, original_params[orig_name], self.current_file, self.fc_parameters | ||
| ) | ||
| self._deleted_parameters.discard(orig_name) | ||
| del self._connection_renames[orig_name] |
Comment on lines
+2386
to
+2397
| for orig_name, new_target in new_rename_map.items(): | ||
| if orig_name in self._connection_renames: | ||
| continue # already handled above | ||
| if orig_name in original_params: | ||
| # Remove the original (un-renamed) parameter if it is still present | ||
| if orig_name in self.current_step_parameters: | ||
| del self.current_step_parameters[orig_name] | ||
| self._deleted_parameters.add(orig_name) | ||
| self.current_step_parameters[new_target] = self._config_step_processor.create_ardupilot_parameter( | ||
| new_target, original_params[orig_name], self.current_file, self.fc_parameters | ||
| ) | ||
| if is_gnss and new_target.endswith("_PROTOCOL"): |
Comment on lines
+2322
to
+2324
| if "rename_connection" in step_info: | ||
| self._refresh_current_step_connection_renames(step_info, variables) | ||
|
|
amilcarlucas
force-pushed
the
inline_component_editor_connection_renames
branch
from
240ee5f to
426735c
Compare
Contributor
☂️ Code Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
…e refresh Add conflict checks in `_refresh_current_step_connection_renames()` so that a rename is skipped (with a warning) when `new_target` already exists in `current_step_parameters` as an unrelated parameter, preventing silent data loss on both the "target changed" and "new rename" branches. Add six BDD-style unit tests in `TestRefreshConnectionRenames` covering: - eval failure leaves state unchanged - normal new rename applies correctly - new rename skipped when target already exists (fix) - rename undone when expression no longer applies - rename retargeted when destination changes - retarget skipped when new target already exists (fix)
Contributor
Test Results 4 files 4 suites 40m 29s ⏱️ Results for commit 2da8a2a. ♻️ This comment has been updated with latest results. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Obey inline component editor connection renames
Checklist
git commit --signoff)Testing
Describe how you tested these changes: