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.

Sign up for GitHub

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

Jump to bottom

Capture HABTM tables #47

Merged
merged 1 commit into from
Sep 2, 2018
Merged

Capture HABTM tables #47

merged 1 commit into from
Sep 2, 2018

Conversation

jdelStrother
Copy link
Contributor

@jdelStrother jdelStrother commented Nov 14, 2017 •
edited
Loading

This is a POC for capturing HABTM relationships (see #38). This feels a bit hacky, but I'm struggling to figure out a non-hacky way.

Translator#instances used to return an array of ActiveRecord objects. It now returns an array of objects, some of which are AR objects, some of which are hashes like {table_name: "recipes_tags", values: {recipe_id: 1, tag_id: 2}}.

It only works on Rails 4.1 or above. In Rails 4.0 and below, the HABTM records are loaded via an inner join which is too hard for me to deal with 😕 .

WDYT?

@nettofarah
Copy link
Contributor

Hey, @jdelStrother.
This looks like a great start!
I had started playing with implementing HABTM too and got something almost good to go.
I'm no longer a maintainer for this project though, so we might need to tag someone from IFTTT here to see if they're interested in supporting this feature.

cc @rf-, @nickyleach, @silvamerica, @spiceee, @trevorturk

@jdelStrother
Copy link
Contributor Author

Anything I can do to help get this merged?

@nettofarah
Copy link
Contributor

hey, @jdelStrother I'm thinking of starting a forked version of Polo and maintain it myself.
Would you be interested in helping me with that as a maintainer?

@jdelStrother
Copy link
Contributor Author

jdelStrother commented Dec 4, 2017 via email

@jdelStrother jdelStrother force-pushed the habtm branch 3 times, most recently from 8d19e49 to 09b97e2 Compare July 10, 2018 14:29
@nettofarah
Copy link
Contributor

hey, @jdelStrother
I got write access to the repo 🎉🎉

Happy to help you drive this across the finish line.
I'm not actively using Polo, but the code looks pretty solid to me.

Should I got ahead and merge the PR?

@jdelStrother
Copy link
Contributor Author

Mmm.. let me double-check it next week. I'm not convinced it's working perfectly on Rails 5.2

@jdelStrother
Copy link
Contributor Author

OK, I've pushed an updated version which merges cleanly with the recent 5.2 fixes. And yes please - go ahead and merge when you have chance.

@jdelStrother
Copy link
Contributor Author

@nettofarah let me know if there's anything I can do to help merge this

@nettofarah
Copy link
Contributor

hey, @jdelStrother
thanks for finding the time to finish up the branch.

I'm gonna go ahead and merge it :)

@nettofarah nettofarah merged commit 6326b1a into IFTTT:master Sep 2, 2018
bessey pushed a commit to bessey/polo that referenced this pull request Feb 21, 2019
Merge branch 'their-master' into lazy-town
49a67f2 
* their-master:
  Capture HABTM tables (IFTTT#47)
  Rails 5.2 support (IFTTT#54)
@jdelStrother jdelStrother deleted the habtm branch April 14, 2020 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jdelStrother @nettofarah