-
Notifications
You must be signed in to change notification settings - Fork 50
custom connection strings to settings.json #1171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
custom connection strings to settings.json #1171
Conversation
7ba4277 to
77f2221
Compare
amilcarlucas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks. Can you please follow pytest_testing_guidelines.md and add high-level BDD tests to the code you added?
18ad507 to
0a7f499
Compare
|
sure ill start working on it |
4b9c0f4 to
5983593
Compare
|
Hi @amilcarlucas , I've added the BDD test for the connection saving logic as requested. P.S. I spotted a typo in the |
0136ad1 to
a553bee
Compare
amilcarlucas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice clean PR. Thanks
ardupilot_methodic_configurator/backend_filesystem_program_settings.py
Outdated
Show resolved
Hide resolved
a553bee to
32fa1d2
Compare
|
Thanks! I've removed the comment, removed the file and updated the test file as requested. |
32fa1d2 to
88d76e0
Compare
|
I noticed the CI failed on a Pylint error Could you please approve the workflows to run again? Thanks! |
88d76e0 to
e294d96
Compare
|
Excelent. I added a commit on top with some fixes. can you sign-off your commit, and leave mine untouched? |
closes ArduPilot#1108 Signed-off-by: Omkar Sarkar <omkarsarkar24@gmail.com>
e294d96 to
ee9f219
Compare
Done! I signed-off my commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
amilcarlucas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good now. Will merge once all CI tests pass. Thanks for contributing
Thank you so much for your patience and for guiding me through this process! This was my first significant open-source contribution, so I learned a lot. |
|
The best workflow is "Test-Driven-Development" (TDD) and writing high-level tests instead of unit tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| expected_result["annotate_docs_into_param_files"] = False # Added by default | ||
| expected_result["gui_complexity"] = "simple" # Added by default | ||
| expected_result["motor_test"] = {"duration": 2, "throttle_pct": 10} # Added by default | ||
| expected_result["connection_history"] = [] # Added for port connection string to save |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is grammatically unclear. Consider rephrasing to: "Added to store connection string history" or "Added for saving port connection strings" for better clarity.
| expected_result["connection_history"] = [] # Added for port connection string to save | |
| expected_result["connection_history"] = [] # Added to store connection string history |
Understood! I will definitely adopt the advice for my next contribution. I’ve saved the Contributing guide to study as well. |
Description
Fixes #1108
Resolves the issue where manually entered connection strings (e.g.,
udp:0.0.0.0:14560) were lost after restarting the application.Changes
Backend (
backend_filesystem_program_settings.py):connection_historylist to the default settings.get_connection_history()andstore_connection()to save/load strings fromsettings.json.Frontend (
frontend_tkinter_connection_selection.py):__init__to load historical connections into the dropdown on startup.add_connectionto save the new string immediately upon user addition.Testing
%APPDATA%\.ardupilot_methodic_configurator\settings.json.