-
-
Notifications
You must be signed in to change notification settings - Fork 397
Allow port, protocol and port settings to be specified in profiles. #2717
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2717 +/- ##
=======================================
Coverage 67.68% 67.68%
=======================================
Files 234 234
Lines 22226 22285 +59
=======================================
+ Hits 15043 15083 +40
- Misses 6001 6017 +16
- Partials 1182 1185 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
LGTM.
It would be awesome to add a test similar to monitor_test.go
That tests for the new profile configs.
Do you think it can be done without too much pain?
This allows to implement the selection logic on more commands.
The previous fixes now let the CLI to produce the correct output.
Thanks to the new tests, I discovered a lot of weird subcases and some old tests that were wrong. 👍 |
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.
Awesome work with the tests. I've done a quick test on my Linux machine too and it works as expected.
🚀
Please check if the PR fulfills these requirements
See how to contribute
before creating one)
our contributing guidelines
UPGRADING.md
has been updated with a migration guide (for breaking changes)configuration.schema.json
updated if new parameters are added.What kind of change does this PR introduce?
port
andprotocol
on a per-profile basis.baudrate
, etc...) on a per-sketch and per-profile basis.What is the current behavior?
Default port and protocol can not be specified per-profile.
Port settings can not be specified either per-sketch or per-profile.
What is the new behavior?
The above features are now allowed inside
sketch.yaml
:Does this PR introduce a breaking change, and is titled accordingly?
Other information
Fix #2640
FIx #2554