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

Delete crdt-example.proto #163

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

pjfanning
Copy link
Contributor

@pjfanning pjfanning commented Aug 11, 2023

I ran sbt interop-tests/test and this file wasn't needed

@mdedetrich
Copy link
Contributor

So I just had a quick glance at the code and I am getting the impression that this is meant to be an example file, in which case its fine to delete but maybe we should make a reference to the file in our documentation somewhere?

@pjfanning
Copy link
Contributor Author

  • it is checked into a test project
  • my impression is that it was meant to be part of the testing of a PR but then it wasn't used
  • if we were to document where you find example protos - what page would it go on? If we have no such page, would we create a page with just this proto?
  • can't we add docs at any time?
  • this file is a potential -1 in our release

Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

this file is a potential -1 in our release

Yeah I am not blocking the deletion of the file, I just find it odd that there is a random source file that isn't even used hence my assumption is that it was meant to be some example file.

Ill approve the PR but can we make an issue on this, i.e. something along the lines of "Figure out what role crdt-example.proto had?"

@pjfanning
Copy link
Contributor Author

I created #164

@pjfanning pjfanning merged commit 3ca2531 into apache:main Aug 11, 2023
@pjfanning pjfanning deleted the remove-unnecessary-proto-file branch August 11, 2023 15:12
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