Docs: signatures for methods returning relations #214

jaredbeck · 2019-10-25T18:55:29Z

I couldn't find any docs on this, so I'm attempting to write some. I'm not at all sure this is correct, but will at least start a conversation. Thanks!

hdoan741 · 2019-10-25T18:57:30Z

Jared, we do support using Bacon::ActiveRecord_Relation. I do agree with need better documentation for it

…

On Fri, Oct 25, 2019 at 11:55 AM Jared Beck ***@***.***> wrote: I couldn't find any docs on this, so I'm attempting to write some. I'm not at all sure this is correct, but will at least start a conversation. Thanks! ------------------------------ You can view, comment on, or merge this pull request online at: #214 Commit Summary - Docs: signatures for methods returning relations File Changes - *M* README.md <https://github.com/chanzuckerberg/sorbet-rails/pull/214/files#diff-0> (14) Patch Links: - https://github.com/chanzuckerberg/sorbet-rails/pull/214.patch - https://github.com/chanzuckerberg/sorbet-rails/pull/214.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#214?email_source=notifications&email_token=AAFH4AMMHJYDSVZ2FKB6ITLQQM6KFA5CNFSM4JFHKU52YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HUOWJKQ>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFH4AIGYMHO72IJ5NAV6QDQQM6KFANCNFSM4JFHKU5Q> .

jaredbeck · 2019-10-25T19:44:22Z

we do support using Bacon::ActiveRecord_Relation ..

Oh, great! Should I update this PR to make that the recommendation?

hdoan741 · 2019-10-25T20:40:25Z

Sure, I think we need it. Hopefully we can resolve the issue like with #211

Thanks for helping with the documentation. Here is some thought I've had:

We need a whole section calling out things sorbet-rails provide at runtime (as opposed to things provided via the static generator). It includes following functionality:

Relation class: Making the relations available at runtime (they are private, the gem make them public)
Model: find_n, first_n, last_n are the methods we added. pluck_to_tstruct as well.
Controller: adding fetch_typed and require_typed

And make sure people put sorbet-rails in a common group in their Gemfile (not development or test only)

jaredbeck · 2019-10-25T23:05:30Z

Here is some thought I've had

Nice! Thanks Harry. I've added your thoughts, and re-organized a little.

codecov-io · 2019-10-25T23:06:12Z

Codecov Report

Merging #214 into master will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
+ Coverage   96.14%   96.22%   +0.08%     
==========================================
  Files         138      138              
  Lines        2228     2228              
==========================================
+ Hits         2142     2144       +2     
+ Misses         86       84       -2

Impacted Files	Coverage Δ
lib/sorbet-rails/model_rbi_formatter.rb	`93.75% <0%> (+3.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12151d4...996b5f4. Read the comment docs.

hdoan741

I provided some feedback. Please let me know what you think

hdoan741 · 2019-10-26T00:02:28Z

README.md

@@ -19,16 +19,14 @@ This gem adds a few Rake tasks to generate Ruby Interface (RBI) files for dynami

 ```
 # -- Gemfile --
-
+# In the default group (same as sorbet-runtime) because of "Features Provided at Runtime" below


I've heard some people don't use sorbet-runtime in the production environment, so I think this part may actually be misleading. Maybe we should say "the group including production environment"

hdoan741 · 2019-10-26T00:04:25Z

README.md

@@ -148,6 +146,22 @@ Since mailing action methods is based on instance methods defined in a mailer cl
 - If not, all the params in the mailing action method will be T.untyped.
 - For return type though, the mailing action method will return `ActionMailer::MessageDelivery` instead of the return type of the instance method.

+## Features Provided at Runtime


I think we should change the current "Type-checking Rails code" to "Static RBI Generation", and move the documentation of run-time features there to this section.

What's needed for this section to work properly is running rake rails_rbi:custom, which will copy the rbi for the provided methods.

jaredbeck · 2019-11-05T15:56:16Z

Updated per comments, please review.

What's needed for this section to work properly is running rake rails_rbi:custom, which will copy the rbi for the provided methods.

I don't understand this comment. Is it something you can add in a future PR?

hdoan741

Thank you :-) I suggested a title so that it's parallel with the "Static RBI Generation" title

hdoan741 · 2019-11-07T19:30:51Z

README.md

@@ -148,6 +147,23 @@ Since mailing action methods is based on instance methods defined in a mailer cl
 - If not, all the params in the mailing action method will be T.untyped.
 - For return type though, the mailing action method will return `ActionMailer::MessageDelivery` instead of the return type of the instance method.

+### Features Provided at Runtime


Suggested change

### Features Provided at Runtime

## Runtime Features

hdoan741 · 2019-11-07T19:41:04Z

Btw, please remove [ci skip] from your commit message. Github checks the PR passes the Travis CI check before it can be merged. Our repo is set up to requires PR passes CI run to merge. Even though this change doesn't make any code change, it's easier to have consistent rule on this.

hdoan741 · 2019-11-07T19:43:59Z

I don't understand this comment. Is it something you can add in a future PR?

Sure, I'll add it later. I'll attempt to explain it still: simply put we need to copy some RBI so that the sorbet knows the signature of the runtime methods added. It's through running the rake task

rake rails_rbi:custom

jaredbeck · 2019-11-07T20:02:32Z

Thanks Harry, changes made.

jaredbeck force-pushed the patch-2 branch from a0907b5 to 57350b0 Compare October 25, 2019 23:03

jaredbeck force-pushed the patch-2 branch 2 times, most recently from 50b1161 to f5fe551 Compare October 25, 2019 23:09

hdoan741 reviewed Oct 26, 2019

View reviewed changes

This was referenced Oct 30, 2019

ActiveRecord_Relation used in types is private #217

Closed

Running tests raises a NameError for sigs receiving or returning an ActiveRecord_AssociationRelation #211

Closed

jaredbeck force-pushed the patch-2 branch from f5fe551 to 07e7b76 Compare November 5, 2019 15:54

hdoan741 approved these changes Nov 7, 2019

View reviewed changes

Docs: signatures for methods returning relations

23eff4e

jaredbeck force-pushed the patch-2 branch from 07e7b76 to 23eff4e Compare November 7, 2019 20:01

Harry Doan added 3 commits November 7, 2019 14:16

Merge branch 'master' into patch-2

ef2343c

Merge branch 'master' into patch-2

f4ba8a4

Merge branch 'master' into patch-2

996b5f4

hdoan741 merged commit 3c2fa85 into chanzuckerberg:master Nov 12, 2019

jaredbeck deleted the patch-2 branch June 15, 2020 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docs: signatures for methods returning relations #214

Docs: signatures for methods returning relations #214

jaredbeck commented Oct 25, 2019

hdoan741 commented Oct 25, 2019 via email

jaredbeck commented Oct 25, 2019

hdoan741 commented Oct 25, 2019

jaredbeck commented Oct 25, 2019

codecov-io commented Oct 25, 2019 •

edited

Loading

hdoan741 left a comment

hdoan741 Oct 26, 2019

hdoan741 Oct 26, 2019

hdoan741 Oct 26, 2019

jaredbeck commented Nov 5, 2019

hdoan741 left a comment

hdoan741 Nov 7, 2019

hdoan741 commented Nov 7, 2019

hdoan741 commented Nov 7, 2019 •

edited

Loading

jaredbeck commented Nov 7, 2019

Docs: signatures for methods returning relations #214

Docs: signatures for methods returning relations #214

Conversation

jaredbeck commented Oct 25, 2019

hdoan741 commented Oct 25, 2019 via email

jaredbeck commented Oct 25, 2019

hdoan741 commented Oct 25, 2019

jaredbeck commented Oct 25, 2019

codecov-io commented Oct 25, 2019 • edited Loading

Codecov Report

hdoan741 left a comment

Choose a reason for hiding this comment

hdoan741 Oct 26, 2019

Choose a reason for hiding this comment

hdoan741 Oct 26, 2019

Choose a reason for hiding this comment

hdoan741 Oct 26, 2019

Choose a reason for hiding this comment

jaredbeck commented Nov 5, 2019

hdoan741 left a comment

Choose a reason for hiding this comment

hdoan741 Nov 7, 2019

Choose a reason for hiding this comment

hdoan741 commented Nov 7, 2019

hdoan741 commented Nov 7, 2019 • edited Loading

jaredbeck commented Nov 7, 2019

codecov-io commented Oct 25, 2019 •

edited

Loading

hdoan741 commented Nov 7, 2019 •

edited

Loading