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

Remove Violations usages from test harness #262

Closed
wants to merge 4 commits into from

Conversation

jchadwick-buf
Copy link
Member

Related to #96.

This should be released separately from (and before) #261, where we can then update the module dependency for protovalidate-testing. That'll avoid the need for tricky business with module syncing.

Copy link

github-actions bot commented Oct 14, 2024

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedOct 16, 2024, 2:45 PM

@jchadwick-buf jchadwick-buf mentioned this pull request Oct 14, 2024
1 task
@jchadwick-buf
Copy link
Member Author

CI failure is completely unrelated, looks like Bazel broke stuff. Guess I'll need to look into that.

@jchadwick-buf
Copy link
Member Author

jchadwick-buf commented Oct 16, 2024

Going slightly bolder, I think we should just remove Violation from the public API altogether. Although I can see the benefit to using Protobuf to define API details across different languages, we now want to expose error details that can't really be expressed in protobuf (essentially live pointers) so the benefit is diminished. Someone may wish to embed the Violation message, but I don't really see why it's any different if they just define their own Violation message versus just using ours. So moving Violation to harness and removing it from the main validate.proto.

@jchadwick-buf jchadwick-buf changed the title Remove Violations usage from test harness Remove Violation and Violations usages from test harness Oct 16, 2024
@jchadwick-buf jchadwick-buf changed the title Remove Violation and Violations usages from test harness Remove Violations usages from test harness Oct 16, 2024
@jchadwick-buf
Copy link
Member Author

Unfortunately this is not as clear cut as I thought. What I did not consider is that Violations is already in use as an error details message. So while we may still want to consider removing it for 1.0, we at least need to do something about some of those existing uses (e.g. in connectrpc/validate-go) so I think now is just not the right time to bite that one off.

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.

1 participant