Simplify re init#1561
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors how parameter default values are stored/updated during initialization and when writing defaults to disk, aligning call sites and tests for consistency.
Changes:
- Renamed
write_param_default_valuestoset_param_default_values_if_differentand updated call sites. - Extended
write_param_default_values_to_fileto support writing defaults into an explicit targetvehicle_dir. - Updated unit tests and documentation to reflect the renamed API and revised initialization behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_data_model_vehicle_project.py | Updates mocks to match the new write_param_default_values_to_file call signature. |
| tests/test_backend_filesystem.py | Renames/updates the unit test to cover set_param_default_values_if_different. |
| tests/test__main__.py | Updates initialization test to use/verify set_param_default_values_if_different. |
| ardupilot_methodic_configurator/backend_filesystem.py | Renames default-setting API and extends default-file writing to allow explicit destination directory. |
| ardupilot_methodic_configurator/main.py | Uses the renamed default-setting API during filesystem initialization. |
| USECASES.md | Updates the initialization steps documentation to include the additional checkbox. |
| self, param_default_values: ParDict, filename: str = "00_default.param", vehicle_dir: str = "" | ||
| ) -> None: | ||
| if self.set_param_default_values_if_different(param_default_values): | ||
| if len(vehicle_dir) == 0: | ||
| vehicle_dir = self.vehicle_dir |
There was a problem hiding this comment.
Using an empty string sentinel plus len(vehicle_dir) == 0 is brittle and non-idiomatic. Prefer vehicle_dir: Optional[str] = None (or a Path | None) and a simple vehicle_dir = vehicle_dir or self.vehicle_dir check; this also avoids a hard failure if a caller ever passes None.
| self, param_default_values: ParDict, filename: str = "00_default.param", vehicle_dir: str = "" | |
| ) -> None: | |
| if self.set_param_default_values_if_different(param_default_values): | |
| if len(vehicle_dir) == 0: | |
| vehicle_dir = self.vehicle_dir | |
| self, param_default_values: ParDict, filename: str = "00_default.param", vehicle_dir: Optional[str] = None | |
| ) -> None: | |
| if self.set_param_default_values_if_different(param_default_values): | |
| vehicle_dir = vehicle_dir or self.vehicle_dir |
| mock_open.assert_called_once_with("/vehicles/my_flight") | ||
| # Assert: extracted defaults written (replacing the template's 00_default.param) | ||
| mock_write.assert_called_once_with(fake_defaults) | ||
| mock_write.assert_called_once_with(fake_defaults, "00_default.param", "/vehicles/my_flight") |
There was a problem hiding this comment.
These assertions depend on positional argument order for a function that now has multiple optional parameters. Using keyword arguments in assert_called_once_with(...) would make the tests more robust against future signature/order changes while still verifying the same behavior.
|
|
||
| # Assert: the extracted defaults — not the template's — are written | ||
| mock_write.assert_called_once_with(fake_defaults) | ||
| mock_write.assert_called_once_with(fake_defaults, "00_default.param", "/vehicles/flight") |
There was a problem hiding this comment.
These assertions depend on positional argument order for a function that now has multiple optional parameters. Using keyword arguments in assert_called_once_with(...) would make the tests more robust against future signature/order changes while still verifying the same behavior.
| mock_write.assert_called_once_with(fake_defaults, "00_default.param", "/vehicles/flight") | |
| mock_write.assert_called_once() | |
| args, kwargs = mock_write.call_args | |
| assert kwargs == {} | |
| assert args[0] == fake_defaults | |
| assert args[1] == "00_default.param" | |
| assert args[2] == "/vehicles/flight" |
…ers dict This should fix a double 00_default.param file inside the "Zip files for forum help" zip file
…nections from FC Parameters, not from template files reference
amilcarlucas
force-pushed
the
simplify_re-init
branch
from
9299151 to
01d1ceb
Compare
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
Coverage Report for CI Build 25024542293Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.1%) to 95.614%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Description
Refactor for consistency
Checklist
git commit --signoff)Testing
Describe how you tested these changes: