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

Adding new adapters #628

Open
TheSmartnik opened this issue Feb 28, 2020 · 12 comments
Open

Adding new adapters #628

TheSmartnik opened this issue Feb 28, 2020 · 12 comments

Comments

@TheSmartnik
Copy link
Contributor

Hi!
Since database_cleaner is moving away from putting all adapters in adapters folder. I was wondering how to contribute new adapters? I guess would be a good idea to add a README entry for that.

@etagwerker
Copy link
Member

@TheSmartnik Definitely a good idea. Do you want to take a stab at it?

The good news is that there are many examples out there now. For example: https://github.com/databaseCleaner/database_cleaner-active_record

I think the most important code will be in this folder: https://github.com/DatabaseCleaner/database_cleaner-active_record/tree/master/lib/database_cleaner/active_record

The section in the README should say a little bit about:

  • What strategies is your new adapter going to support?
  • What methods do you have to define in your new adapter?

Anything else, @botandrose?

@TheSmartnik
Copy link
Contributor Author

@etagwerker thank you for a quick reply! Sure, would love to do that. One thing is unclear, though.
When adding a new adapter:

  1. Do I create a folder adapters in database_cleaner repository and you create a separate gem and rept after the pr is merged.
  2. Do I have to create my own repo and transfer it to you
  3. Something else?

@botandrose
Copy link
Contributor

@TheSmartnik Yes, thank you for pointing this out! Now that adapters can be made and maintained without involving changes to database_cleaner core, we should absolutely document how this can be done in the README. Your perspective as an outsider trying to do this will be extremely helpful in creating this documentation. Thank you! I think @etagwerker's suggestion above is a really good place to start. Any bumps you hit or questions you have while doing this will be really valuable.

To answer your question above: in theory, there would be no actual need to involve database_cleaner core at all, in the form of PRs, etc. You can create your adapter repo, and maintain it separately, just as the split-off adapter gems are.

I suppose there's the question of having it be "blessed" or "adopted" into the DatabaseCleaner Github organization, but I think that question can be deferred until after the adapter is up and running and in use.

@etagwerker
Copy link
Member

@TheSmartnik What @botandrose said is correct:

  1. Do I create a folder adapters in database_cleaner repository and you create a separate gem and rept after the pr is merged.

You don't need to submit a PR to database_cleaner with the adapter code, you can create your own repository. Once it's up and running, you can submit a PR to database_cleaner so that we can list it here: https://github.com/databaseCleaner/database_cleaner/#list-of-adapters

  1. Do I have to create my own repo and transfer it to you

You don't need to transfer it to the DatabaseCleaner organization. In the future we can consider adding it to our organization.

I suppose there's the question of having it be "blessed" or "adopted" into the DatabaseCleaner Github organization, but I think that question can be deferred until after the adapter is up and running and in use.

Definitely. We can discuss this in the future.

@botandrose
Copy link
Contributor

Thinking about this some more, perhaps database_cleaner-core can package some kind of test suite that adapters can include that verifies that they're adhering to the contract that we will define in the README. Something like:

require "database_cleaner/rspec"

describe DatabaseCleaner::ActiveRecord do
  it_behaves_like "a database_cleaner adapter"
end

This could verify (at the very least) that the described constant supplies a list of strategies, a default strategy, and that each strategy has all the right methods defined. Thoughts?

@TheSmartnik
Copy link
Contributor Author

@botandrose I really like the idea. Do you want to do it? I can implement that in a separate PR before implementing the README update

@botandrose
Copy link
Contributor

Awesome, yes, please go ahead!

@TheSmartnik
Copy link
Contributor Author

I took a look at the current adapters and it seems that maybe it would be a good idea to add a cli command for generating adapter. Something like

database_cleaner generate adapter elasticsearch

What do say?

@botandrose
Copy link
Contributor

@TheSmartnik I think that's a really interesting idea, and worth considering!

It would add some complexity to the project, though, so let's wait and see where things are at after we fully document the process for building a new adapter manually. Looking at that, there may be opportunities for us to simplify the process to the point where its so simple to do so that introducing a generator would be overkill. I think this would be the best outcome, if its possible, but let's wait and see.

At least, those are my thoughts on the matter. @etagwerker what do you think?

@TheSmartnik
Copy link
Contributor Author

@botandrose I've started working on an README, but have found out that since v2 things changed a bit. For once, database_cleaner-core isn't yet released, but all future adapters should depend on it and not on database_cleaner. So I thought maybe it's a good idea to wait with it until v2 is released. What do you think?

@botandrose
Copy link
Contributor

@TheSmartnik Personally, I think having a good README story for adapter writers is something that I want to have when v2 is released. Also, this is our last chance to break things until v3 (which hopefully won't be for a long time), so if there are small things we can break now to make adapter writing simpler and easier, I'd love to do it before v2.

In short, I consider your efforts here vital for v2! Consider me at your disposal for this. :)

@botandrose
Copy link
Contributor

@TheSmartnik Hiya! I'm getting ready to move onto documenting (and hopefully improving) the story for adapter writers, ahead of the upcoming v2 release. If you have anything to share, even if its just a Work in Progress, I'd love to see it!

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

No branches or pull requests

3 participants