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

added neo4j adapter #128

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

pdipietro
Copy link

Hi all,
after signaling an error on the wiki, I decided to add a new adapter.
Here you can find the code, BUT there is still the unresolved error I signaled in the Issue #127:

  • $ rails s
    pdipietro@CUC-N4J-V4-0-0:~/gsn$ rails s
    simple_token_authentication/adapters/active_record_adapter
    simple_token_authentication/adapters/mongoid_adapter
    simple_token_authentication/adapters/neo4j_adapter
    bin/rails:6: warning: already initialized constant APP_PATH
    /home/pdipietro/gsn/bin/rails:6: warning: previous definition of APP_PATH was here
    Usage: rails COMMAND [ARGS]

Please, look at the error and then decide if you can merge this fork

@gonzalo-bulnes
Copy link
Owner

Hi @pdipietro,

The Neo4j support addition is coordinated from #98, please be sure to comment about your PR there so we stay in sync. (Let's comment here about specificities of you PR and there about the Neo4j support.)

A few comments about your PR:

First of all, thank you!

As mentionned in the README, tests can't be missing. They ensure each new feature is both robust and documented.

The adapters interface allows to extends support for ORM/ODM/OxM in a standard way (see #97 please, the discussion documents the intentions behind the code). I documented that standard way while adding support for the Mongoid ODM, and you can find it described step by step in #124. My intention was making the protocol easy to follow, and hopefully easy to understand even if you're not familiar with test-driven development. Don't hesitate to ask if something is unclear. : )

The last one is a detail: I use to enforce some naming conventions for branches to keep the repository graph readable, and I would suggest this one to be named add-neo4j-support ; )

Finally, I've never seen your error. I can't check the Travis build from my phone, but I

@gonzalo-bulnes
Copy link
Owner

(...continuing) I'm pretty sure that only the test which documents the default model_adapters value is failing. If it occurs to be correct, let's discuss about the error in the issue you opened.

@ianks
Copy link

ianks commented Nov 12, 2014

Looks good, you should change the name back in the Gemspec to gonzalo-bulnes, though.

@pdipietro
Copy link
Author

@ianks: done!

@ianks
Copy link

ianks commented Nov 21, 2014

Some more notes:

  1. I don't think neo4j-devise needs to actually be a dependency in the Gemfile.
  2. Can you squash down all the commits related to Gemfile modifications?
  3. Tests for the adapter. @gonzalo-bulnes can clarify a bit more on this, are there integration tests with the actual databases? I did not notice this if so.

@gonzalo-bulnes gonzalo-bulnes added the new feature This pull request adds a new feature. label Jan 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This pull request adds a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants