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

Update state machine #32

Merged
merged 9 commits into from
Sep 23, 2021
Merged

Update state machine #32

merged 9 commits into from
Sep 23, 2021

Conversation

gitbuda
Copy link
Member

@gitbuda gitbuda commented Aug 12, 2021

No description provided.

@gitbuda gitbuda added the enhancement New feature or request label Aug 12, 2021
@gitbuda gitbuda self-assigned this Aug 12, 2021
@gitbuda
Copy link
Member Author

gitbuda commented Aug 15, 2021

Closes #28

  • Added InTransaction status.
  • Checking preconditions with match + Result seems quite good to me because it's contained in one block. There is a difference between https://github.com/memgraph/pymgclient/blob/master/src/connection-int.c#L77 ("internal" error handling) and https://github.com/memgraph/pymgclient/blob/master/src/cursor.c#L156 ("public" error handling). It seems to me that Rust's pattern matching + Result combines everything in one place. Long story short, I would leave that as is (the Rust structure), but we should discuss 😄 to make lasting changes.
  • There is some difference in the behavior at this point, that's for the discussion as well.
  • Connection::pull_all is a private method used in one place, it could be inlined, but that's more-less equivalent.
  • In the Python case, the cursor is there because of https://www.python.org/dev/peps/pep-0249/. In the Rust case, that clearly defined API doesn't exist. I would keep that simple for now.

@gitbuda gitbuda marked this pull request as ready for review August 15, 2021 08:39
src/connection/tests.rs Outdated Show resolved Hide resolved
src/connection/tests.rs Outdated Show resolved Hide resolved
src/connection/tests.rs Outdated Show resolved Hide resolved
@gitbuda
Copy link
Member Author

gitbuda commented Aug 18, 2021

@antaljanosbenjamin I think this is finally ready for review 😄

Copy link
Contributor

@antaljanosbenjamin antaljanosbenjamin left a comment

Choose a reason for hiding this comment

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

I created two issues (#34 and #35) regarding to our discussion yesterday. There are two minor comments from me, but it looks really nice. The status management become much readable.

src/connection/mod.rs Outdated Show resolved Hide resolved
src/connection/mod.rs Outdated Show resolved Hide resolved
gitbuda and others added 2 commits September 23, 2021 17:14
Co-authored-by: János Benjamin Antal <antaljanosbenjamin@users.noreply.github.com>
Co-authored-by: János Benjamin Antal <antaljanosbenjamin@users.noreply.github.com>
@gitbuda gitbuda merged commit 3bb92f0 into master Sep 23, 2021
@gitbuda gitbuda deleted the update-state-machine branch September 23, 2021 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants