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

Issue-2748 - Add ByteBuffer Hex init & write #2837

Merged
merged 10 commits into from
Sep 30, 2024

Conversation

ali-ahsan-ali
Copy link
Contributor

Add ability for below two functions
ByteBuffer(hexEncodedBytes: "68 65 6c 6c 6f 20 77 6f 72 6c 64 0a")
buffer.writeHexEncodedBytes("68656c6c6f20776f726c640a")

Motivation:

#2748

@ali-ahsan-ali ali-ahsan-ali force-pushed the Issue-2748-ByteBuffer-Hex-Initialiser branch from 28cf942 to 4621870 Compare August 11, 2024 06:34
Sources/NIOCore/ByteBuffer-aux.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-hex.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-hex.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-hex.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-hex.swift Outdated Show resolved Hide resolved
@ali-ahsan-ali ali-ahsan-ali force-pushed the Issue-2748-ByteBuffer-Hex-Initialiser branch 3 times, most recently from 2a7cede to 3e2b725 Compare August 24, 2024 11:02
@ali-ahsan-ali ali-ahsan-ali force-pushed the Issue-2748-ByteBuffer-Hex-Initialiser branch from fde9321 to e1fb499 Compare September 7, 2024 05:55
@ali-ahsan-ali
Copy link
Contributor Author

@weissi please take a look when you find the time

@Lukasa Lukasa added the semver/minor Adds new public API. label Sep 9, 2024
Sources/NIOCore/ByteBuffer-aux.swift Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-aux.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-hex.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-hex.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-hex.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-hex.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-hex.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Ok awesome, this is getting a lot closer. Only a few little notes here.

Sources/NIOCore/ByteBuffer-aux.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-aux.swift Outdated Show resolved Hide resolved
public struct HexDecodingError: Error {
let kind: HexDecodingErrorKind

public enum HexDecodingErrorKind {
Copy link
Contributor

Choose a reason for hiding this comment

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

This enum needs to be private, or we can't safely use this type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting internal? Private would mean this would be impossible (to my understanding) to initialise (atleast the way I have done so).

Copy link
Contributor

@Lukasa Lukasa Sep 17, 2024

Choose a reason for hiding this comment

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

Private should be fine, you'd use public static lets on the parent to access the fields:

public struct HexDecodingError: Error {
    private let kind: Kind

    private enum Kind {
        case invalidHexLength
        case invalidCharacter
    }
    
    public static let invalidHexLength = HexDecodingError(kind: .invalidHexLength)
    public static let invalidCharacter = HexDecodingError(kind: .invalidCharacter)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks will find time to implement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made HexDecodingError equatable. Makes sense to be able to see what kind of error you have and if it is not by making kind public, then I think the alternative is the equatable comformance. It also helps with tests. LMK what you think.

Sources/NIOCore/ByteBuffer-hex.swift Outdated Show resolved Hide resolved
@ali-ahsan-ali ali-ahsan-ali force-pushed the Issue-2748-ByteBuffer-Hex-Initialiser branch from 70052c4 to eea3a60 Compare September 17, 2024 11:12
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Fantastic, this is looking really good. A pair of very minor textual tweaks and then I'm happy. Thanks for sticking with this!

Sources/NIOCore/ByteBuffer-aux.swift Outdated Show resolved Hide resolved
Sources/NIOCore/ByteBuffer-hex.swift Outdated Show resolved Hide resolved
@ali-ahsan-ali
Copy link
Contributor Author

Fantastic, this is looking really good. A pair of very minor textual tweaks and then I'm happy. Thanks for sticking with this!

Doing a bit of travelling. Actually flying to London for the swift conference. Will find time to push the changes.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Awesome, I'm happy with this! Well done getting through the gauntlet again @ali-ahsan-ali, it's really good that you were able to stick with it. Much appreciated ✨ .

@Lukasa Lukasa enabled auto-merge (squash) September 30, 2024 17:00
@Lukasa Lukasa merged commit 67dd9ca into apple:main Sep 30, 2024
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants