-
Notifications
You must be signed in to change notification settings - Fork 26
RSDK-8892: Remove ProtoType #301
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
Merged
lia-viam
merged 18 commits into
viamrobotics:main
from
lia-viam:feature/remove-proto-type
Oct 4, 2024
Merged
Changes from 14 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
7ff05a7
first attempt conan ci job
lia-viam b04fc36
remove git
lia-viam f34e007
Merge branch 'main' into feature/conan-builds-ci
lia-viam 95f407e
comment out repo owner
lia-viam c5bc9a6
global install
lia-viam be04812
always set build = missing
lia-viam 6f1720e
Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk
lia-viam 6b5d948
remove prototype and replace with protovalue
lia-viam bd23b2d
no longer compile proto_value into the tests
lia-viam 91e8070
remove unused boost/variant includes
lia-viam 328bb29
fix examples and tests
lia-viam 9bb3c9b
try to revert clang-format disagreement
lia-viam 8b998d3
more format reverts
lia-viam 395590c
Merge branch 'main' into feature/remove-proto-type
lia-viam 7195172
more explicit param name
lia-viam bfefaaa
Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk
lia-viam 4dc6c67
Merge branch 'main' into feature/remove-proto-type
lia-viam 59ce752
Merge branch 'feature/remove-proto-type' of github.com:lia-viam/viam-…
lia-viam File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is aw this patern in
base/impl.cppabove too, and sort of shrugged it off, but, it now that I see it twice it feels a little chatty. IsProtoValuemissing some affordance for this sort of pattern? Wouldstd::optional(or a polyfill for it) simplify things? ShouldProtoValuehave some monadic APIs (and_then, transform, or_else), etc?This, btw, is what motivated my slack ping about polyfills / C++17 since I feel like
optionalprobably plays a role in such an API.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.
Yeah it was bugging me as well that this kept coming up, being that it's the one scenario where we end up more verbose than the previous code.
I personally am a big fan of optionals and monadic operations but in this case we might be better off with a pointer API. For example Boost.JSON has these, and we could spell them as
try_get<T>or similar, sinceif_boolstrikes me as a bit idiosyncraticIn any case definitely in agreement that this would be nice to simplify, will create a ticket.