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

use base64 crate for encoding #29

Merged
merged 1 commit into from
Mar 5, 2022

Conversation

danieleades
Copy link
Contributor

use the base64 crate, rather than hand-rolling this

@@ -40,6 +40,7 @@ deflate = { version = "0.9.1", optional = true }
sha1 = { version = "0.6.0", features = ["std"] }
tempfile = "3.2.0"
futures = "0.3.17"
base64 = "=0.20.0-alpha.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is an alpha release, though the author suggests that the API and implementation are stable. I've pinned the version to mitigate any risk here.

see marshallpierce/rust-base64#174 (comment)

Comment on lines +8 to +10
const ENGINE: FastPortable =
match Alphabet::from_str("0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-_") {
Ok(alphabet) => FastPortable::from(&alphabet, fast_portable::PAD),
Err(_e) => unreachable!(),
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this construction is needed since Result::unwrap is not (yet) const

@sytsereitsma
Copy link
Owner

Unit test is failing. The padding characters have changed from 00 to ==. Still seems to work, so please fix the unit test to use the new padding characters.

@danieleades
Copy link
Contributor Author

Unit test is failing. The padding characters have changed from 00 to ==. Still seems to work, so please fix the unit test to use the new padding characters.

ah rats. I think I only ran the unit tests in the base64_plantuml module....

should be working now

@sytsereitsma
Copy link
Owner

Hi Daniel. Can you resolve the conflicts after my merge of your other PR?

@danieleades
Copy link
Contributor Author

rebased. this is ready for review

@sytsereitsma
Copy link
Owner

Working down the line... You'll have to do some more rebasing I'm afraid.

@sytsereitsma sytsereitsma merged commit f5c8752 into sytsereitsma:master Mar 5, 2022
@danieleades danieleades deleted the base64 branch March 5, 2022 21:50
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