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

Refactored BasicPullResponseHandler to use an explicit state machine #661

Merged
merged 1 commit into from
Dec 13, 2019

Conversation

gjmwoods
Copy link
Contributor

This PR refactors BasicPullResponseHandler to make its state machine explicit.

@gjmwoods gjmwoods force-pushed the 4.0-refactor-pull-state-machine branch 2 times, most recently from a40de29 to 73d7b84 Compare December 11, 2019 13:07
Copy link
Contributor

@zhenlineo zhenlineo left a comment

Choose a reason for hiding this comment

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

Looks pretty good.

@@ -267,13 +229,217 @@ private void dispose()
this.summaryConsumer = null;
}

protected Status status()
protected State getState()
Copy link
Contributor

Choose a reason for hiding this comment

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

The driver uses naming style void status(Status s), and Status status(). This is because when we are tying to give a unified API across all languages, getXX and setXX does not apply in many languages such as python or Js. So we decided we use getter and setters in this driver without get and set. I would suggest to change the name back so that we keep the naming style in the whole codebase to be consistent.

}

protected void status( Status status )
protected void setState( State state )
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for the setter.

@gjmwoods gjmwoods force-pushed the 4.0-refactor-pull-state-machine branch from 73d7b84 to 6645f5b Compare December 12, 2019 16:44
@zhenlineo zhenlineo merged commit bc105be into neo4j:4.0 Dec 13, 2019
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