Skip to content

JWTKit 5, FoundationEssentials-only + more#253

Merged
MahdiBM merged 7 commits intomainfrom
mmbm-jwt-kit-5-and-more
Oct 12, 2024
Merged

JWTKit 5, FoundationEssentials-only + more#253
MahdiBM merged 7 commits intomainfrom
mmbm-jwt-kit-5-and-more

Conversation

@MahdiBM
Copy link
Collaborator

@MahdiBM MahdiBM commented Oct 11, 2024

No description provided.

@MahdiBM MahdiBM requested review from 0xTim and gwynne as code owners October 11, 2024 17:48
@MahdiBM MahdiBM requested a review from ptoffy October 11, 2024 17:48
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Few comments to improve things but LGTM

@@ -1,5 +1,6 @@
import DiscordBM
import JWTKit
import struct Foundation.Date
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be FoundationEssentials.Date to avoid linking all of Foundation?

@@ -1,4 +1,4 @@
import Foundation
import Foundation /// Need to import the whole thing for the `String.split()` function call below?
Copy link
Member

Choose a reason for hiding this comment

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

It should be in essentials unless it relies on ICU to ensure unicode is split correctly but I don't think that's the case, I thought that was all reimplemented in Swift for this reason

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what exactly is going on but things don't compile without importing the whole thing.
Something about CharacterSet I think, for one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah right IIRC CharacterSet is still not ported over? There were some issues around it in the main OpenAPIGenerator repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is my own comment from a few months ago. Apparently it was accurate:
apple/swift-openapi-runtime#107 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the problem was CharacterSet: e96f991

@MahdiBM MahdiBM enabled auto-merge (squash) October 12, 2024 10:47
@MahdiBM MahdiBM merged commit 7ce484f into main Oct 12, 2024
@MahdiBM MahdiBM deleted the mmbm-jwt-kit-5-and-more branch October 12, 2024 10:57
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