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.

Sign up for GitHub

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

Jump to bottom

AVRO-3904: [Rust] Change the return type of SchemaCompatibility::can_read() from bool to AvroResult<()> #2585

Closed
wants to merge 1 commit into from
Closed

AVRO-3904: [Rust] Change the return type of SchemaCompatibility::can_read() from bool to AvroResult<()> #2585

wants to merge 1 commit into from

Conversation

martin-g
Copy link
Member

AVRO-3904

API break

What is the purpose of the change

  • Do not panic when schemata are incompatible. Return Err and let the user deal with it.

Verifying this change

  • Run the build and test

Documentation

  • Does this pull request introduce a new feature? no

@github-actions github-actions bot added the Rust label Nov 15, 2023
@martin-g
Copy link
Member Author

The unit tests still fail! Needs debugging!

@martin-g martin-g force-pushed the avro-3904-SchemaCompatibility--can_read-should-return-Result branch from 2b372c8 to 3ef3b96 Compare November 15, 2023 14:24
@marcosschroh
Copy link
Contributor

marcosschroh commented Nov 15, 2023 •
edited
Loading

This looks really similar to https://github.com/apache/avro/compare/main...marcosschroh:avro:fix/compatibility-read-when-field-is-subset?expand=1 @martin-g I pushed literal an hour ago, I stopped to solve an issue in my personal work and I see this... I have never seen such a thing... incredible

@martin-g martin-g force-pushed the avro-3904-SchemaCompatibility--can_read-should-return-Result branch from 3ef3b96 to 22617e4 Compare November 15, 2023 14:28
@marcosschroh
Copy link
Contributor

Next time I will make sure to create a PR a microsecond after pushing to my fork to avoid people stealing things. This one would have been my first contribution to a project using rust as I am a newie in rust , thanks a lot @martin-g really appreciate it.

@martin-g
AVRO-3904: [Rust] Change the return type of SchemaCompatibility::can_…
3b8f6fc 
…read() from `bool` to AvroResult<()>

Do not panic when schemata are incompatible. Return Err and let the user
deal with it.

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@martin-g martin-g force-pushed the avro-3904-SchemaCompatibility--can_read-should-return-Result branch from 22617e4 to 3b8f6fc Compare November 15, 2023 14:35
@martin-g
Copy link
Member Author

Next time I will make sure to create a PR a microsecond after pushing to my fork to avoid people stealing things. This one would have been my first contribution to a project using rust as I am a newie in rust , thanks a lot @martin-g really appreciate it.

Sorry for stealing your joy! :-)
I had free time and decided to work on this...

@martin-g
Copy link
Member Author

@marcosschroh You can still review and approve the PR! Thanks!

@marcosschroh
Copy link
Contributor

marcosschroh commented Nov 15, 2023 •
edited
Loading

As I mentioned: I am a newbie in rust, so another person with more experience should look at it. Despite my lack of knowledge on rust, yesterday we agreed in the jira ticket that I had to send a PR and then you (@martin-g in theory with more experience than me in rust) would look at it (not the other way around). Involve another developer with rust experience to get feedback.

PS: If you still have some honesty left, you know what you have to do.

@martin-g
Copy link
Member Author

I have no time for drama!
I will close the PR and let you open a new one!

@martin-g martin-g closed this Nov 16, 2023
@martin-g martin-g deleted the avro-3904-SchemaCompatibility--can_read-should-return-Result branch November 16, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Rust
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martin-g @marcosschroh