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

Add readme directions on migrating to Tapioca #473

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

peternixey
Copy link

Motivation

On initially installing Tapioca I had no idea that it needed a migration in order to work from an existing Sorbet installation. This signals that need directly from the Readme

Implementation

Updated readme

Tests

n/a

On initially installing Tapioca I had no idea that it needed a migration in order to work from an existing Sorbet installation. This signals that need directly from the Readme
@ghost ghost added the cla-needed label Sep 1, 2021
Copy link
Collaborator

@Morriar Morriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

README.md Outdated Show resolved Hide resolved
@peternixey
Copy link
Author

I was actually thinking exactly the same thing but didn't want to be presumptuous. Let me update the PR accordingly

peternixey and others added 2 commits September 1, 2021 15:14
Co-authored-by: Alexandre Terrasa <583144+Morriar@users.noreply.github.com>
I swapped a little of the info around on the frontpage so that the two types of installation were next to each other, the commands and flags were placed next to each other and there was a little more context to the intro at the start of the page
@peternixey
Copy link
Author

Hi @Morriar, I've moved the migration details into the main readme. I also tweaked a couple of other things to make it more consistent and to focus the start on the installation and the end on the details:

  • two types of installation were next to each other and both termed "installation" (to discourage an exsting sorbet install to first install and then migrating - which won't work)
  • the commands and flags were placed next to each other
  • added a little more context to the intro at the start of the page
  • moved the section on "Manual gem requires" to the bottom of the page

Apologies if the changes look confusing - I'm certain everything is in there, just re-ordered!

Copy link
Collaborator

@Morriar Morriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, thanks a lot for taking care of this. Our README desperately needed some love ❤️

I'm not sure a hard separation between fresh Sorbet project and projects using srb rbi is relevant. Many sections / steps are common to both routes and we'll end up repeating a lot of content.

What do you think about merging the two approaches but marking some steps as optional if they are related to sorbet-rails or srb rbi?

README.md Outdated Show resolved Hide resolved
README.md Outdated
As yet, no gem exports type information in a consumable format and it would be a huge effort to manually maintain such an interface file for all the gems that your codebase depends on. Thus, there is a need for an automated way to generate the appropriate RBI file for a given gem. The `tapioca` gem, developed at Shopify, is able to do exactly that to almost 99% accuracy. It can generate the definitions for all statically defined types and most of the runtime defined types exported from Ruby gems (non-Ruby gems are not handled yet).

When you run `tapioca sync` in a project, `tapioca` loads all the gems that are in your dependency list from the Gemfile into memory. It then performs runtime introspection on the loaded types to understand their structure and generates an appropriate RBI file for each gem with a versioned filename.

## Manual gem requires
Tapioca helps simplify your setup too. Gems such as `sorbet-typed`, `sorbet-rails` are ways to provide accurate typing information for DSLs and gems. However, Tapioca aims to provide the complete tooling to generating RBIs for gems and DSLs, and does not require being combined with other gems, such as `sorbet-rails`, which can be removed from your gemfile.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Said like that it sounds like Tapioca does not provide accurate typing information contrary to other solutions?

README.md Outdated

For example, suppose you are using the class `BetterHtml::Parser` exported from the `better_html` gem. Just doing a `require "better_html"` (which is the default require) does not load that type:
## Installing Tapioca in a project with no existing RBI files (i.e. a fresh Sorbet project)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put back the section ## How does tapioca compare to "srb rbi gems" ? here, before jumping to the installation procedure.

README.md Outdated
=> true
[5] pry(main)> BetterHtml::Parser
=> BetterHtml::Parser
### 1. Add this line to your application's `Gemfile`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the numbering from the headers? It will make adding/removing/reordering sections more tedious.

README.md Outdated Show resolved Hide resolved
README.md Outdated

Done! Your application should be all set and type checking should pass.

## How does tapioca compare to "srb rbi gems" ?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to document all of this here, we should also add a section explaining how to keep the RBIs up-to-date when the Gemfile changes and the files related to DSLs change.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

1. Run `bin/tapioca generate`. This generates RBIs for the gems in your application
2. Try to run the type checker (`bundle exec srb tc`)
3. If you notice that definitions are missing for gems, you might need to add the gem for which those definitions belong to in `sorbet/tapioca/require.rb`. Before manually adding requires, try running `bin/tapioca require` so that Tapioca can figure out the requires. If this still does not work, proceed to write manual requires
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's link to the Manual gem requires section.

README.md Outdated
This step is the iterative process to generate the RBIs necessary for your application's gems. This part may vary depending on your setup.

1. Run `bin/tapioca generate`. This generates RBIs for the gems in your application
2. Try to run the type checker (`bundle exec srb tc`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't all of this also be done for fresh installs?

peternixey and others added 2 commits September 7, 2021 13:18
Apply minor suggestions (prior to looking at larger ones)

Co-authored-by: Alexandre Terrasa <583144+Morriar@users.noreply.github.com>
- merged the two installation paths into one path with optional sections
- added links to sections as suggested in code review
- reduced a lot of annotative text into code blocks to make more readable
@peternixey
Copy link
Author

Hi @Morriar - sorry for the delayed response, I was away on holiday.

I went through your changes, all of which seemed very reasonable. I've done a complete revamp of the page here to combine the two different flows into one and then to add in the links and clarity you suggested:

https://github.com/peternixey/tapioca/blob/main/README.md

I wrote it all on the assumption that most people will find Tapioca after they find Sorbet and so the primary use-case is for an existing sorbet installation. You may know the reality of that better than me though!

BTW one thing I did was compress a lot of the text down into code blocks which I think makes it easier to scan and consume - you may feel differently though.

I also wonder whether on a document like this you might spend more time giving me kind feedback than actually doing the edits yourself. If you'd like to take the doc as it is (or as it was) and edit it however you see fit I will not be in the slightest offended and it will probably take you less time than writing me kind tweak-suggestions ;)

So let me hand this over to you and it is yours to merge / bin / use-for-inspriation as helpful!

Best wishes,
Peter

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I can understand a lot of users will arive to tapioca after knowing about Sorbet I think our primary flow needs to for users that didn't setup anything yet and extract the migration "guide" to a different document linked in the README.

We can still link to the document before we explain the "pristine flow" but having both flows in the same document when users will either fall in one or another category could be confusing.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@peternixey
Copy link
Author

@rafaelfranca you guys obviously know your audience much better than me but I’m just interested in the logic as to why most people would come to Tapioca with a pristine setup.

Surely one finds out about Tapioca after trying Sorbet and then realising that it has its limitations. I could understand if Tapioca was part of the official Sorbet setup guide / package but as far as I know it’s not.

To assuage my curiosity, what’s the context in which a user would discover Tapioca prior to discovering Sorbet?

@peternixey
Copy link
Author

I’ve just realised that of course Ruby 3 uses RBI files too so presumably that would be the other (and potentially main) use case - is that right?

@rafaelfranca
Copy link
Member

Ruby 3 uses RBS files, not RBIs, so that is another migration setup.

To assuage my curiosity, what’s the context in which a user would discover Tapioca prior to discovering Sorbet?

Inside Shopify mostly.

But my interest of moving out the migration steps is mainly because of the perspective of the library itself. As a library, Tapioca has no knowledge on your setup, so it assumes you have a pristine setup, that is the path we mostly care about. Of course how users land here will vary, and that is fine, we should document some of those cases. I just don't think having everything in the README will make it easier to read. The README should only include the pristine way. From there we can link to other pages, sometimes maintained by different people, for the various setups that people might have before arriving here.

@peternixey
Copy link
Author

Hi @rafaelfranca and thanks for your answers. I'd like to clarify what would be most helpful to you guys from this next. I

I'm getting conflicting messages from your side though so I'd prefer to either wrap up or hand it over. @Morriar had originally suggested combining the Sorbet-based installation with the pristine installation and so that's the way that I took it.

I'm not worried if you'd prefer to leave this but if it's helpful then there's no point in wasting the few hours that I've spent revising it. If it's a hassle tell me then I'll leave it, if it's an asset then let's make the most of the couple of hours I've spent on it so far:

Can I suggest the following course of action:

  1. You tell me whether or not you'd like to tweak what I've got
  2. You decide whether you'd prefer to do it or you'd like me to (for small changes it's far more sensible for you to do them)
  3. You decide on one person who's code reviewing so as to avoid bouncing between different points of opinion.

FWIW I would suggest that the bulk of people outside of Shopify will find Tapioca via Sorbet so it's probably a big thing to factor into your instructions and accounting for it will hugely boost people's initial impressions of Tapioca.

I can offer you another hour of my time but only take it if you actually want this PR otherwise let's just park it.

Best
Peter

@bmulholland
Copy link

Typo: dela -> deal

@peternixey peternixey requested a review from a team as a code owner July 14, 2022 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants