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

Support #find_by, #find_by!, #where, #order, and #limit #56

Closed
wants to merge 5 commits into from
Closed

Support #find_by, #find_by!, #where, #order, and #limit #56

wants to merge 5 commits into from

Conversation

chrisfrank
Copy link
Collaborator

This PR closes #19 and makes the following behavior possible:

Tea.where("Type" => "Green").where("Price" => 10).order("Name" => "asc").limit(1)
Tea.find_by("Name" => "Rooibos Vanilla")

I still need to update the README and changelog, but wanted to make sure this approach looks okay to you, @sirupsen. I'm happy with it so far, but have some questions:

  • Should #where accept raw Airtable formula components, in addition to the conditions hash it currently accepts? Like Tea.where("Type => "Green").where("{Price} < 30")?
  • To support chaining, #where, #order, and #limit all return an instance of Airrecord::Relation, an Enumerable that only calls Table#records after you try do something with it, like each, map, to_a, etc. In 2.0, should the has_many API also return a Relation instead of an Array, so you could call Tea.find(id).brews.where("Temperature" => 90)?
  • I didn't see anything in the ActiveRecord code that looked like it would make query chaining easier than a plain Ruby class, but I definitely don't want to write callbacks or validations from scratch! What do you think of https://www.rubydoc.info/github/apotonick/hooks for callbacks, and maybe https://github.com/dry-rb/dry-validation for validations? For the sake of the Ruby devs who write web apps without ActiveRecord (all twelve of us), I'm reluctant to add a dependency as large as ActiveRecord. Maybe that's naive of me 🤷‍♂️
  • I updated these tests against a live Airtable base to cover the Relation methods. What do you think about adding a few live tests to the Airrecord repo, behind an optional ENV flag or something? I'd like to able to test against a live repo before every release.

@chrisfrank chrisfrank mentioned this pull request Jan 12, 2019
Closed
@chrisfrank
Copy link
Collaborator Author

Updated this today with clearer variable names

@chrisfrank chrisfrank requested a review from sirupsen January 12, 2019 20:43
@sirupsen
Copy link
Owner

Hey @chrisfrank, will try to get to this soon—sorry, the beginning of a new year is typically busy :)

@chrisfrank
Copy link
Collaborator Author

No worries :)

@jmgarnier
Copy link

Hello, thank you to both for the gem and this PR. I was wondering if you will close this PR or integrate it :)
I can give you a hand if you're busy to review and release the gem

@chrisfrank
Copy link
Collaborator Author

Thanks for resurfacing this, @jmgarnier. I defer to @sirupsen, but when I look at this now, I'm seeing a major change to a stable gem, and one that duplicates functionality already available via the filter, sort, and max_records keywords in Table#all. I'm also troubled that although this proposed API is more like ActiveRecord's, it's not (and can't be) exactly like ActiveRecord's. Having used this branch in an app that I later moved to a database, I think I would have been better off just writing Airtable formulas to begin with.

I'd propose scaling this back to just find_by and find_by!, which would solve the original issue and make for a much smaller change.

@chrisfrank
Copy link
Collaborator Author

@jmgarnier, I'd love to hear more about which features from this PR would be helpful to you. Is it the method-chaining style? Or that you could filter records via plain Ruby hashes instead of using Airtable formulas? Something else?

I don't yet see a way to support chaining that addresses the worries I expressed yesterday, but I actually think we could support filtering with hashes. @sirupsen, what if we adjusted the filter keyword in Table#all to accept a Hash as well as a string? It would treat strings as raw formulas like it always has, and convert hashes to formulas under the hood.

Also, @jmgarnier, I'm realizing I never acknowledged your kind offer to help work on this :) Thank you very much for being willing to pitch in. Once we figure out how to proceed, I'd be happy to review a PR!

@Meekohi
Copy link
Collaborator

Meekohi commented Jan 8, 2021

This got snoozed a pretty long time unfortunately. Anyone feel strongly about reviving it, or shall we close it?

@sirupsen
Copy link
Owner

sirupsen commented Jan 9, 2021

👍 Feel free to close

@Meekohi Meekohi closed this Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sirupsen sirupsen Awaiting requested review from sirupsen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add find_by_id method
4 participants
@chrisfrank @sirupsen @jmgarnier @Meekohi