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

Base32 #29

Merged
merged 7 commits into from Mar 23, 2021
Merged

Base32 #29

merged 7 commits into from Mar 23, 2021

Conversation

adam-fowler
Copy link
Collaborator

Given the last Base32 PR seems to have died. Here is mine

@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #29 (7208165) into main (9c7648c) will increase coverage by 0.33%.
The diff coverage is 91.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
+ Coverage   91.32%   91.66%   +0.33%     
==========================================
  Files           2        2              
  Lines         173      276     +103     
==========================================
+ Hits          158      253      +95     
- Misses         15       23       +8     
Impacted Files Coverage Δ
Sources/ExtrasBase64/Base64.swift 91.32% <91.22%> (+51.32%) ⬆️
Sources/ExtrasBase64/Base32.swift 92.23% <92.23%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c7648c...7208165. Read the comment docs.

Copy link
Member

@slashmo slashmo left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time, @adam-fowler! This looks great 👍

Comment on lines +42 to +43
var string = "AAAAAAAAAAAAAAAAA"
string.makeContiguousUTF8()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var string = "AAAAAAAAAAAAAAAAA"
string.makeContiguousUTF8()
let string = "AAAAAAAAAAAAAAAAA"

Is there a reason to call makeContiguousUTF8 here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't necessary. I copied Fabian tests for Base64 and he has it there.


func testBase32DecodingWithPoop() {
XCTAssertThrowsError(_ = try Base32.decode(bytes: "💩".utf8)) { error in
XCTAssertEqual(error as? Base32.DecodingError, .invalidCharacter(240))
Copy link
Member

Choose a reason for hiding this comment

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

💩👍

@adam-fowler
Copy link
Collaborator Author

@slashmo I noticed Fabian had prepared a branch which moves code about to make it more inline with having two baseN implementations. ie The base64 code is inside the Base64.swift file and not Chromium.swift. Do you want me to merge that in as well? Would look cleaner

@gwynne
Copy link
Member

gwynne commented Mar 10, 2021

@adam-fowler I would certainly like to see that part merged in, for sure.

@adam-fowler
Copy link
Collaborator Author

@gwynne Base64 code has been moved into Base64.swift to make it consistent

@gwynne gwynne merged commit 97237cf into swift-extras:main Mar 23, 2021
@gwynne gwynne mentioned this pull request Mar 23, 2021
@adam-fowler adam-fowler deleted the base32 branch March 23, 2021 12:25
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.

None yet

3 participants