Update gRPC API to create a new sketch #1498
Conversation
|
|
|
No idea on what's wrong with "Check internationalization" task.. |
4ntoine
force-pushed
the
1456_sketch-new
branch
3 times, most recently
from
3384213
to
ca1ba0f
Hi @4ntoine. Thanks for your pull request. You can learn about syncing the internationalization data with your changes here: Please let me know if you have any questions or problems. |
|
Yeah, originally i started with this design and then i decided that the
caller does not have to be that smart and specify the path per every call
but that receive the path instead. Also if we design it to accept the path
every time we will be unable to do 'sketch list' as they can be in
different dirs every time. So i'd prefer to have a single dir for them as a
setting.
What do you think?
вт, 12 окт. 2021 г., 17:46 Silvano Cerza ***@***.***>:
… ***@***.**** commented on this pull request.
------------------------------
In docs/configuration.md
<#1498 (comment)>:
> @@ -9,6 +9,7 @@
- `downloads` - directory used to stage downloaded archives during Boards/Library Manager installations.
- `user` - the equivalent of the Arduino IDE's ["sketchbook" directory][sketchbook directory]. Library Manager
installations are made to the `libraries` subdirectory of the user directory.
+ - `sketches` - directory used to store user sketches if called via gRPC.
You can also specify a directory for the Sketch, I'd expect a gRPC client
to always do that.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1498 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGOWT4UTGUFSWEPMJ5SR7TUGQUZDANCNFSM5FPK5DIQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
That's fine - i will just use existing path, will update the MR
вт, 12 окт. 2021 г., 20:03 Cristian Maglie ***@***.***>:
… ***@***.**** commented on this pull request.
------------------------------
In docs/configuration.md
<#1498 (comment)>:
> @@ -9,6 +9,7 @@
- `downloads` - directory used to stage downloaded archives during Boards/Library Manager installations.
- `user` - the equivalent of the Arduino IDE's ["sketchbook" directory][sketchbook directory]. Library Manager
installations are made to the `libraries` subdirectory of the user directory.
+ - `sketches` - directory used to store user sketches if called via gRPC.
Second, for cli it works as expected - it creates a sketch in the current
dir (and this MR does not change that).
👍🏼
But over gRPC there is no current directory. Should i just use user
directory instead of newly introduced sketches?
yes, thanks. We are already using user_dir to store sketches (also the
IDE lists sketches from the user_dir) I would not add another setting in
the mix.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1498 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGOWT2U652BTFNLUYNXE5TUGRE3FANCNFSM5FPK5DIQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
…e "user" dir for sketches created via gRPC
|
Rebased to upstream master, tested from cli and via gRPC - seems to be ok |
|
I left some suggestions. A pretty important thing, please use our library go-paths-helper to handle paths and not All in all a great contribution, I also like your idea of a Also sorry if we're a bit slow on reviews but we got tons of things to do. |
4ntoine
force-pushed
the
1456_sketch-new
branch
3 times, most recently
from
5a27b81
to
10b7cac
4ntoine
force-pushed
the
1456_sketch-new
branch
2 times, most recently
from
98547d9
to
45904d2
|
@silvanocerza Ready for the review |
Approved!
I forgot to tell you to change the CLI command sketch new to call directly the gRPC function so I made a commit on it.
Thanks again @4ntoine, really apprreciate your contribution.
I'll handle the merging.
Please check if the PR fulfills these requirements
before creating one)
our contributing guidelines
UPGRADING.mdhas been updated with a migration guide (for breaking changes)Updates gRPC (daemon) API to support creating a new sketch
Not implemented (#1456)
Implemented and available via gRPC
titled accordingly?
No breaking changes
Still requires some polishing (fix i18n) - at least 1 string needs to be translated (error message).
No tests were added in the repo, but i tested it with Dart gRPC client manually instead.
See how to contribute
The text was updated successfully, but these errors were encountered: