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

Experimenting with Sorbet #361

Merged
merged 16 commits into from
Nov 19, 2021
Merged

Experimenting with Sorbet #361

merged 16 commits into from
Nov 19, 2021

Conversation

yob
Copy link
Owner

@yob yob commented Sep 5, 2021

Add a sorbet type check step to CI, and distribute rbi files with the gem for downstream users who use sorbet.

I'm not sure if I'll keep this long term, but I'm interested in playing with sorbet and this gem is a good playground.

A few features of this change:

  • the sorbet/ directory was built with bundle exec srb init, as described in sorbet docs
  • the rbi/pdf-reader.rbi file was built with bundle exec parlour. I assume I'll need to re-run this command anytime methods or classes are added/removed/changed. Maybe changes will need to be carefully added to git with -p as well? We'll see
  • the parlour gem raises a NoMethodError when parsing lib/pdf/reader/parser.rb. I haven't worked out why yet, so for now that file is excluded from the auto rbi generation
  • all types are stored outside the code, to avoid any runtime dependency on sorbet. Most downstream users aren't using sorbet, and should gain a dependency on it when they upgrade pdf-reader
  • the types are distributed in the gem, which should mean downstream users who run sorbet should see that it knows the types pdf-reader expects
  • I've got nearly every file in lib/ to types: true, and a handful to typed: strict
  • Most methods don't have proper signatures yet, they have the not-so-useful sig { params(T.untyped).returns(T.untyped) }
  • There's some CI environment shenanigans to avoid loading any sorbet on ruby < 2.3, which sorbet refuses to install on

@yob yob force-pushed the sorbet branch 6 times, most recently from 96b3025 to edca009 Compare September 5, 2021 13:09
@yob yob force-pushed the sorbet branch 3 times, most recently from e48fed9 to 8e980f5 Compare October 23, 2021 07:23
@yob yob force-pushed the sorbet branch 3 times, most recently from 54496e6 to a0e470d Compare November 19, 2021 12:20
Shipping the RBI file with the gem like this means downstream users of
pdf-reader who also use sorbet will know the types pdf-reader expects
for its public API.

Storing the sigs outside the source files also means there's no need to
add sorbet-runtime as a dependency of the gem, and downstream users of
pdf-reader who do not use sorbet will see no change.

I generated the RBI file with parlour:

    bundle exec parlour

I assume I'll have to regenerate it over time as the methods on each
class evolve. That sounds like a huge hassle, but this is an experiment
so lets see how it goes.

The parlour config file ignore lib/pdf/reader/parser.rb because I get a
NoMethodError if I leave it in.
Recent versions of sorbet require this. The rbi file was automatically
updated to meet this requirement with:

    bundle update sorbet
    bundle exec srb tc -a
Newer versions of sorbet introduce changes that will break type checks
and require some intervention. This forces a fixed version so all
developers and the CI environment will get consistent behaviour, and we
can update intentionally when we're ready.
srb rbi gems changed these sigils for me 🤷‍♂️
sorbet doesn't work with ruby < 2.3, but that's OK. We don't really need
it to.

We mainly need sorbet in two places:

* the dev environment
* a single CI step with a version of ruby that sorbet supports, to run
  the type checking
@yob yob merged commit feb9203 into main Nov 19, 2021
@yob yob deleted the sorbet branch November 19, 2021 12:35
yob added a commit that referenced this pull request Jan 17, 2022
This was originally skipped in #361 because I was getting an unreachable
code error. I found it was possible to avoid that by providing a type
annotation for the `mode` variable of the state machine instead of hard
coding it to `:none`.

Sadly, I can't do the type annotation with `T.let()` because sorbet
isn't a runtime dependency of pdf-reader, but I can hack it in via a sig
in the RBI file if I make the initial state part of the method params.
yob added a commit that referenced this pull request Jan 17, 2022
This was originally skipped in #361 because I was getting an unreachable
code error. I found it was possible to avoid that by providing a type
annotation for the `mode` variable of the state machine instead of hard
coding it to `:none`.

Sadly, I can't do the type annotation with `T.let()` because sorbet
isn't a runtime dependency of pdf-reader, but I can hack it in via a sig
in the RBI file if I make the initial state part of the method params.
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.

None yet

1 participant