Skip to content

Introduce default traits#1995

Merged
FranzBusch merged 8 commits into
apple:mainfrom
madsodgaard:traits
Mar 6, 2026
Merged

Introduce default traits#1995
FranzBusch merged 8 commits into
apple:mainfrom
madsodgaard:traits

Conversation

@madsodgaard

Copy link
Copy Markdown
Contributor

Follows up on the discussion in #1993 and introduces the traits:

  • BinaryDelimitedStreams
  • FieldMaskSupport

@madsodgaard madsodgaard changed the title Introduce traits Introduce default traits Mar 2, 2026
Comment thread Sources/SwiftProtobuf/BinaryDelimited.swift

@thomasvl thomasvl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looking back at #1505, I believe PathDecoder, PathVisitor, and Message+FieldMask.swift should probably also get gated.

Is there a good way to test with each of these disable to confirm we've got things properly gated?

Comment thread Tests/SwiftProtobufTests/Test_AsyncMessageSequence.swift
Comment thread Package.swift Outdated
@thomasvl

thomasvl commented Mar 3, 2026

Copy link
Copy Markdown
Collaborator

Is there a good way to test with each of these disable to confirm we've got things properly gated?

Any suggestions for this part so we don't accidentally break these?

@FranzBusch

Copy link
Copy Markdown
Member

Any suggestions for this part so we don't accidentally break these?

We can setup CI that tests with no traits or other trait combinations if we want to.

@thomasvl

thomasvl commented Mar 3, 2026

Copy link
Copy Markdown
Collaborator

Any suggestions for this part so we don't accidentally break these?

We can setup CI that tests with no traits or other trait combinations if we want to.

That seems like it might be worth while. And maybe adding something to make check that checks without them. Is there something we can reference to setting this up? I'm guessing it can land after the fact.

@madsodgaard

Copy link
Copy Markdown
Contributor Author

@thomasvl I extended CI to test the different combinations of traits, let me know if this is suitable 👍

@thomasvl thomasvl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@FranzBusch how's this looking to you? Since you have more experience with traits.

Comment thread .github/workflows/build.yml
Comment thread .github/workflows/build.yml
@madsodgaard madsodgaard requested a review from FranzBusch March 5, 2026 07:55
Comment thread .github/workflows/build.yml Outdated
@FranzBusch

Copy link
Copy Markdown
Member

This LGTM to me besides this minor nit. If @thomasvl and @allevato are good with this being the only default trait to exist until a next major then I am good with moving forward here.

@allevato

allevato commented Mar 5, 2026

Copy link
Copy Markdown
Collaborator

If @thomasvl and @allevato are good with this being the only default trait to exist until a next major then I am good with moving forward here.

LGTM. Nothing else immediately jumps out at me as something we might want to be able to disable for the time being.

@thomasvl thomasvl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nothing else comes to mind.

@thomasvl

thomasvl commented Mar 5, 2026

Copy link
Copy Markdown
Collaborator

@FranzBusch someone will need to update the expected checks before this can land. And do you want semver/minor on this?

@FranzBusch FranzBusch added the 🆕 semver/minor Adds new public API. label Mar 6, 2026
@FranzBusch

Copy link
Copy Markdown
Member

Updated the checks and added minor to indicate the introduction of the default trait

@FranzBusch FranzBusch merged commit aac2b60 into apple:main Mar 6, 2026
18 of 19 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.

4 participants