-
Notifications
You must be signed in to change notification settings - Fork 192
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
Add serde support to Blob type #2647
Conversation
@@ -30,3 +40,122 @@ impl AsRef<[u8]> for Blob { | |||
&self.inner | |||
} | |||
} | |||
|
|||
#[cfg(all(aws_sdk_unstable, feature = "serde-serialize"))] |
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.
what about refactoring this into a module so you can feature gate it once?
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.
Gotcha.
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.
By the way, why do you have both base64
and base64-simd
?
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.
We should only be using base64-simd
, I think. base64
is there for this bench comparing base64
to base64-simd
.
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.
implementation looks fine, would be nice to clean up the feature gating a little bit.
Side note—it would be good to add a test to aws-smithy-types additional ci to verify these other deps don't end up in the tree when the cfg gate isn't enabled
Thank you for the suggestion. You don't really have a way to check whether a struct implements a trait on runtime. Maybe check if you can compile when the features are not enabled? Let me think about it for a bit. |
I added a test that checks whether the traits are properly feature gated by checking whether compilation would succeed. It tries to compile these pieces of code. fn main() {
serde_json::to_string(&ReplaceDataType::default());
} fn main() {
let dt: Result<ReplaceDataType, _> = serde_json::from_str("some_str");
} If |
12b47e3
to
65c2bea
Compare
I refactor the code. |
@@ -0,0 +1,228 @@ | |||
/* |
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.
although very clever, I think we can do something much simpler by adding something like adding:
cargo tree -e no-dev | grep serde
to the additional-ci
script in this directory
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.
That's sounds simpler.
I updated.
Let me know if I didn't understand you correctly.
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.
I added these code to the script.
# checks whether the features are properly feature-gated
res=$(cargo tree -e no-dev | grep serde)
if [ "$res" != "" ]; then
exit 1
fi
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.
OK, I didn't know that grep gives you an error when it can't find a match.
I fixed it up
e9d47be
to
30f9987
Compare
1b9b08e
to
6f220f4
Compare
set -e | ||
|
||
echo "### Checking for duplicate dependency versions in the normal dependency graph with all features enabled" | ||
cargo tree -d --edges normal --all-features | ||
|
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.
It should be working now.
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.
Looks good. Just some minor suggestions.
Co-authored-by: John DiSanti <[email protected]>
Co-authored-by: John DiSanti <[email protected]>
Thank you for the contribution! ❤️ |
Motivation and Context
This is a child PR of #2616
The changes that this PR introduces is same as the ones that were merged to
unstable-serde-support
branch before.Initially, we tried to make commit to unstable-serde-support branch and merge changes one by one in small PRs. However, in order to make it up to date with the main branch, we would need to go through a large PR of over 700 files.
Thus, I decided to create individual PRs that commits directly to
main
branch.Description
serde
support toBlob
Testing
Checklist
NA
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.