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

feat: Relax Message Debug trait bound #1147

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tottoto
Copy link
Contributor

@tottoto tottoto commented Sep 4, 2024

Relaxes Message's Debug trait bound.

BREAKING CHANGE: trait Debug was a supertrait of trait Message. This is no longer required by prost. If your code relies on trait Debug being implemented for every impl Message, you must now explicitly state that you require both Debug and Message. For example: where M: Debug + Message

@tottoto tottoto force-pushed the relax-message-debug-trait-bound branch from 9fb1f4f to 6bac6c3 Compare September 4, 2024 01:22
@tottoto tottoto force-pushed the relax-message-debug-trait-bound branch from 6bac6c3 to 28e633b Compare September 4, 2024 01:33
Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

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

  • Can you explain why this is useful?
  • Is this an API breaking change?

@tottoto
Copy link
Contributor Author

tottoto commented Sep 4, 2024

Can you explain why this is useful?

When the Debug implementation is not needed, we can use prost_build::Config::skip_debug() to skip implementing Debug. However as the Messages trait requires Debug implementation, it is needed to implement dummy implementation to implement Message when it is not actually needed. Relaxing Message's Debug trait bound requirement removes this.

Is this an API breaking change?

I think this is a breaking change. Previously, when a value's type had a Message bound imposed on it, we could write code that relied on that value implementing Debug, after this change, we will get a compilation error and will need to explicitly state Debug.

@caspermeijn
Copy link
Collaborator

Can you explain why this is useful?

When the Debug implementation is not needed, we can use prost_build::Config::skip_debug() to skip implementing Debug. However as the Messages trait requires Debug implementation, it is needed to implement dummy implementation to implement Message when it is not actually needed. Relaxing Message's Debug trait bound requirement removes this.

I see, so skip_debug was already useful to write your own impl Debug. With this change you can use skip_debug to not implement Debug at all. That makes sense.

Is this an API breaking change?

I think this is a breaking change. Previously, when a value's type had a Message bound imposed on it, we could write code that relied on that value implementing Debug, after this change, we will get a compilation error and will need to explicitly state Debug.

I understand. I will prepare this for the next breaking release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants