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 unnecessary panic for unreachable code #26

Merged
merged 3 commits into from
Apr 27, 2021

Conversation

dejankos
Copy link
Contributor

This PR removes unnecessary panic for unreachable code
and unifies behavior for lazy and eager data fetching.

Although std::unreachable!() could be used here and
serve as self explanatory there is no reason for keeping
this behavior in a lib API that returns a Result type.

Test is rewritten since as far as I can see it was just a c/p where
no data was fetched in loop but code would panic on first
fetchone fn invocations since result was explicitly set to None.

@gitbuda gitbuda added enhancement New feature or request good first issue Good for newcomers labels Apr 26, 2021
@gitbuda gitbuda self-requested a review April 26, 2021 16:53
@gitbuda
Copy link
Member

gitbuda commented Apr 26, 2021

@dejankos Nice! I'll check the code a bit later but overall seems reasonable. Thank you!! 🥇

src/connection/mod.rs Outdated Show resolved Hide resolved
src/connection/tests.rs Outdated Show resolved Hide resolved
@gitbuda
Copy link
Member

gitbuda commented Apr 27, 2021

cargo fmt changed the two suggestions, so please apply them if that makes sense. As soon as that gets fixed, the PR is ready to be merged.

There is a failing test:
Screenshot from 2021-04-27 06-18-39
But I've noticed that a couple of days ago, I think it's nothing related to this change. There has to be another PR fixing that.

dejankos and others added 2 commits April 27, 2021 08:02
Co-authored-by: Marko Budiselić <mbudiselicbuda@gmail.com>
Co-authored-by: Marko Budiselić <mbudiselicbuda@gmail.com>
@dejankos
Copy link
Contributor Author

@gitbuda Ah yes sorry about fmt, changes accepted.
For that test since attributes size is asserted instead of attribute names I can only guess it's related to running Memgraph version.

Copy link
Member

@gitbuda gitbuda left a comment

Choose a reason for hiding this comment

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

Great! Thank you very much! I'll merge soon.

@gitbuda gitbuda merged commit d00caf7 into memgraph:master Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants