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

docs(sqlcommenter_rails/README): update marginalia fork to modulitos #99

Merged
merged 1 commit into from
Mar 2, 2022

Conversation

modulitos
Copy link
Contributor

@modulitos modulitos commented Feb 28, 2022

This PR updates the sqlcommeter_rails readme so that it references the modulitos fork instead of the glebm fork. The modulitos fork makes the same changes as the glebm fork, except that it rebases the latest changes from the master branch of the marginalia repo, so that we'll have support for Rails 6.

For reference, here's the original commit where this README was originally updated:
70d62f0

Here's the PR to merge the modulitos fork into the master branch of marginalia:
basecamp/marginalia#130

I am working with the marginalia maintainers to get that PR merged in. When I do, I will update this README so that we can remove this extra step of using a marginalia fork :)

Note:
Sorry about the duplicate PRs that I created earlier - I was having trouble getting the Google CLA checks to pass.

@google-cla
Copy link

google-cla bot commented Feb 28, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@sjs994
Copy link
Collaborator

sjs994 commented Mar 1, 2022

Thanks @modulitos, Looks good.
Just want to check any tentative ETA for the merging the marginalia fork into basecamp ? It would be more user friendly in that way.

We can merge this PR if you think basecamp/marginalia#130 will take long time.

@sjs994 sjs994 self-requested a review March 1, 2022 18:18
@modulitos
Copy link
Contributor Author

@sjs994 Thanks for the quick reply.
I'm not sure what the ETA is for basecamp/marginalia#130, but I'll be checking in with the maintainers and anyone else who might be involved with that project. I know some forums where I might be able to find some reviewers and get eyes on the PR.

In the meantime, I think it makes sense to merge in this PR. This will save new users from having to troubleshoot Rails 6 issues, and re-doing my efforts in rebasing the existing fork with the master branch of Marginalia. I'll be sure to update this README as soon as I get the fork merged into master :)

@sjs994
Copy link
Collaborator

sjs994 commented Mar 2, 2022

I'm not sure what the ETA is for basecamp/marginalia#130, but I'll be checking in with the maintainers and anyone else who might be involved with that project. I know some forums where I might be able to find some reviewers and get eyes on the PR.

Thanks for the reply. Understandable on the timeline part.

Will merge this PR.

@sjs994 sjs994 merged commit 0ed325c into google:master Mar 2, 2022
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