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

Refactoring, cleanup and generalization #319

Merged
merged 6 commits into from Dec 2, 2018

Conversation

klobuczek
Copy link
Member

Fixes #
This is a neutral PR that does not introduce any new functionality. It does some refactoring and cleanup and exposes hook methods that are necessary for the java driver adaptor to work properly.
This pull introduces/changes:

  • adds close methods on driver and session
  • packs shared spec example and a spec helper into the gem for reuse by adaptors in separate gems

Pings:
@cheerfulstoic
@subvertallchris

@coveralls
Copy link

coveralls commented May 28, 2018

Coverage Status

Coverage increased (+1.5%) to 86.15% when pulling 9843f1c on klobuczek:java_driver into e4a2639 on neo4jrb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 84.528% when pulling ae1ec57 on klobuczek:java_driver into 5eaa804 on neo4jrb:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 84.528% when pulling ae1ec57 on klobuczek:java_driver into 5eaa804 on neo4jrb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 84.528% when pulling ae1ec57 on klobuczek:java_driver into 5eaa804 on neo4jrb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 84.528% when pulling ae1ec57 on klobuczek:java_driver into 5eaa804 on neo4jrb:master.

Copy link
Contributor

@cheerfulstoic cheerfulstoic left a comment

Choose a reason for hiding this comment

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

Generally good, just a couple of questions

.travis.yml Outdated
@@ -16,7 +16,7 @@ jdk: oraclejdk8
rvm:
- 2.4.2
- 2.1.10
- jruby-9.1.14.0
- jruby-9.2.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Something that I've been doing is putting something like jruby-9 so that it just gets whatever the newest version of jRuby 9.x.x.x is

@@ -1,7 +1,10 @@
require 'active_support'
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally we've been requireing just the parts of active_support that are needed. For example

@@ -12,6 +13,7 @@ class CypherSession
module Adaptors
class Bolt < Base
include Adaptors::HasUri
include Adaptors::Schema
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this extraction still allow the gem to work for HTTP on versions of Neo4j before 3.0.0? You couldn't query for everything through Cypher back then...

Copy link
Member Author

Choose a reason for hiding this comment

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

This extraction is only for the purpose of enabling code sharing between bolt and java_driver adapter. The http adapter is not affected by this.

@@ -102,6 +102,10 @@ def connected?
!!@requestor
end

def supports_metadata?
version(nil) >= '2.1.5'
end
Copy link
Contributor

Choose a reason for hiding this comment

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

String comparison of versions can break down. See this

@klobuczek
Copy link
Member Author

@cheerfulstoic could we move ahead on that? It is not ideal to point our production to forks and keep synching the fork with upstream.

@klobuczek klobuczek merged commit 11b5b2d into neo4jrb:master Dec 2, 2018
@klobuczek klobuczek deleted the java_driver branch December 2, 2018 14:27
@klobuczek klobuczek restored the java_driver branch December 2, 2018 14:28
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.

None yet

3 participants