-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Update chip-tool to use the InvokeCommand API #10611
Update chip-tool to use the InvokeCommand API #10611
Conversation
Answering #10498 (comment) here. I disagree with this and I would be happy to hear the rationale behind it, especially given the fact that #10498 contains a template to fill in nested arguments such as list of struct that contains list from json. |
CI will likely be red since |
PR #10611: Size comparison from 1cb7d96 to 6cd4b2e 14 builds
12 builds
|
Passing JSON string at the command-line as an argument feels quite un-wieldy to me from a developer-friendliness standpoint:
If the user enters the wrong JSON string, or the wrong type/field-name above, would there be schema validation provided that would inform them of what they did wrong? A potential alternative based on Python:
There is type-safety built in above, as well as rich help text we can provide for each generated type to guide the user to filling in the arguments. This is in addition to the fact that we have an interactive shell with history that can be used to send previous commands, print out responses/errors, parse out the response and access individual fields in it, etc. |
I am not arguing about the fact that passing JSON string is better than using python with interactive shell. The only thing I'm saying is that there are some developers using |
6cd4b2e
to
00d3f64
Compare
PR #10611: Size comparison from c12fdbe to 00d3f64 8 builds
Increases above 1.0% from c12fdbe to 00d3f64:
14 builds
|
00d3f64
to
e3cb094
Compare
PR #10611: Size comparison from ca22841 to e3cb094 5 builds
Increases above 1.0% from ca22841 to e3cb094:
15 builds
2 builds
2 builds
10 builds
|
@jepenven-silabs @jmartinez-silabs @Damian-Nordic @LuDuda Please take a look? |
e3cb094
to
348b7f9
Compare
PR #10611: Size comparison from 0a3f98a to 348b7f9 1 build (for telink)
Increases above 1.0% from 0a3f98a to 348b7f9:
15 builds (for k32w, linux, p6, qpg)
6 builds (for efr32, mbed)
12 builds (for esp32, nrfconnect)
|
Problem
chip-tool
command line does not use theInvokeCommand
API for most commandsChange overview
chip-tool
to use the newInvokeCommand
API for most commandsTesting
I have manually tested it.