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

Sequel integration #367

Closed
wants to merge 2 commits into from
Closed

Sequel integration #367

wants to merge 2 commits into from

Conversation

palin
Copy link

@palin palin commented Mar 9, 2018

No description provided.

@delner delner self-requested a review March 9, 2018 16:35
@delner delner added integrations Involves tracing integrations community Was opened by a community member feature Involves a product feature labels Mar 9, 2018
@delner
Copy link
Contributor

delner commented Mar 9, 2018

Hey @palin , thanks for putting this together! Love seeing support for new libraries. We've been particularly interested in adding Sequel for some time now, too, so this is great.

I'll take a look through this when you feel it's ready for review. In the mean time, since we do require tests for contributions, I'd suggest adding some RSpec tests that exercise this new integration.

@palin
Copy link
Author

palin commented Apr 5, 2018

Hi @delner, sorry didn't see your comment before.
The code works but is dirty. There are probably lines which should be removed.
We will try to add some rspec tests to it soon.
Thanks

@delner
Copy link
Contributor

delner commented Apr 5, 2018

@palin Sorry for catching this late, but I was looking through old PRs and found this one #171 which seems to already add Sequel integration.

Does this PR do something that this one doesn't? If it does, we should maybe rebase this PR on top of that one and any missing functionality. Otherwise I think we should close this one.

@palin
Copy link
Author

palin commented Apr 9, 2018

@delner Thank you for your response. We looked through the #171 and it looks good. Do you have any ETA for merging that PR?

@delner
Copy link
Contributor

delner commented Apr 9, 2018

@palin I don't yet, other to say that it's on our queue, albeit unscheduled. I'll can let you know as things develop.

@palin
Copy link
Author

palin commented Apr 10, 2018

OK, please let me know then. Thanks

@palazzem palazzem added this to the 0.13.0 milestone Apr 16, 2018
@delner
Copy link
Contributor

delner commented Apr 18, 2018

Hey @palin , we started rebasing/updating #171 with the intention to merge it soon. Going to close this PR in favor of that one. Feel free to add any feedback you might have about Sequel on that, and we can tackle it there.

@delner delner closed this Apr 18, 2018
@delner delner removed this from the 0.13.0 milestone May 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants