Skip to content

Add a test for a custom OutputProtocol-conforming type#235

Open
itingliu wants to merge 5 commits intoswiftlang:mainfrom
itingliu:demo-custom-output
Open

Add a test for a custom OutputProtocol-conforming type#235
itingliu wants to merge 5 commits intoswiftlang:mainfrom
itingliu:demo-custom-output

Conversation

@itingliu
Copy link
Copy Markdown
Contributor

@itingliu itingliu commented Apr 8, 2026

  • Implement unimplemented functions. Replace fatalError wtih comments explaining why the function is no-op
  • Add a test for custom output type
  • Remove the comment that discourages providing a custom Output type

@itingliu itingliu requested a review from iCharlesHu as a code owner April 8, 2026 22:28
Copy link
Copy Markdown
Contributor

@iCharlesHu iCharlesHu left a comment

Choose a reason for hiding this comment

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

Thank youuu!

var array: [UInt8] = []
for index in 0..<span.byteCount {
array.append(span.unsafeLoad(fromByteOffset: index, as: UInt8.self))
span.withUnsafeBytes { ptr in
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(not a issue) This TODO was more about switching to a String initializer that takes a Span directly. I don't think that API is ready yet unfortunately.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah I know the intention. I was looking for it so eagerly the other day too. For now at least this change stops us from potentially reallocating the array... still need a Span one though unfortunately.

#endif

@Suite(.serialized)
struct ProtocolConformanceTests {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(nit) Any particular reason to create a new test file instead of just reusing IntegrationTests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm used to having a struct for one particular kind of test, so I created another one here. Honestly I didn't look closely what IntegrationTest contains. I can move back if you prefer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ahh not at all. It's probably a good idea to split them up anyways.

@itingliu itingliu force-pushed the demo-custom-output branch from 1374cae to 69bdd32 Compare April 9, 2026 15:49
@itingliu
Copy link
Copy Markdown
Contributor Author

itingliu commented Apr 9, 2026

@swift-ci please test

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