Skip to content
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

implementing padding #3

Merged
merged 4 commits into from
Feb 10, 2024
Merged

implementing padding #3

merged 4 commits into from
Feb 10, 2024

Conversation

owendevita
Copy link
Contributor

@owendevita owendevita commented Feb 6, 2024

This patch implements the random padding surrounding the BSON packet. The one included unit test ensures a proper wrapping and unwrapping of our packet. Future needs may warrant further unit testing for invalid data.

Signed-off-by: Owen De Vita owen.n.devita@gmail.com

@owendevita owendevita closed this Feb 6, 2024
@owendevita owendevita reopened this Feb 6, 2024
Copy link
Owner

@amyipdev amyipdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better, still some work to do. Feel free to just push commits onto your existing branch

src/padding.rs Outdated Show resolved Hide resolved
src/padding.rs Outdated Show resolved Hide resolved
src/padding.rs Outdated Show resolved Hide resolved
src/padding.rs Outdated Show resolved Hide resolved
src/padding.rs Outdated Show resolved Hide resolved
src/padding.rs Outdated Show resolved Hide resolved
Applied changes including:
Inlining the generation of random bytes
Avoiding panic on .take()
Working directly on the slice
Using with_capacity

Improved readability with constants for future updates if needed.
Copy link
Owner

@amyipdev amyipdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks almost good, just two minor things (really one in two places) and then we're ready to merge!

src/padding.rs Outdated Show resolved Hide resolved
src/padding.rs Outdated Show resolved Hide resolved
Improve the readability of the code by implementing the constant variables more consistently.
Copy link
Owner

@amyipdev amyipdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Reviewed-by: Amy Parker amy@amyip.net

@amyipdev amyipdev merged commit ae85c82 into amyipdev:main Feb 10, 2024
1 check passed
@owendevita owendevita deleted the padding-branch branch February 10, 2024 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants