-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
Move tapioca args to config; exclude unnecessary gems #16691
Conversation
# These conflict with the rbi files provided by sorbet: | ||
- json | ||
- msgpack | ||
# These aren't needed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't noticeably speed up typecheck updates
Given this, I think this will just result in more work needed to keep this exclude list updated and correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there's some way to automatically audit this to make sure it's up-to-date. Removing 60k lines from the codebase seems useful too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test that validates the list of exclusions are all actual dependencies. (Though, I don't feel too strongly about greatly expanding the list here, should folks still have reservations.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too worried about excluding things that aren't dependencies anymore. I'm more concerned about excluding dependencies that become needed for some reason. Is there an automatic way to determine whether a dependency “is needed”?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can imagine a brew typecheck
feature that can identify an incorrect exclude
, based on the namespace of a type error, but that's probably more trouble than it's worth.
I've asked in the #tapioca channel of the sorbet slack to see if excluding unreferenced dependencies is something other folks bother with, I will update if I hear anything back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use this list on another project and: it becomes pretty obvious when things are needed because brew typecheck
will fail until RBI files are added.
10a838d
to
89b5914
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dduugg!
# These conflict with the rbi files provided by sorbet: | ||
- json | ||
- msgpack | ||
# These aren't needed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use this list on another project and: it becomes pretty obvious when things are needed because brew typecheck
will fail until RBI files are added.
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Moving tapioca args to a config both simplifies code and allows for using tapioca tooling outside of the
typecheck
command. I also noticed we're compiling a bunch of gems that aren't being used in ruby. Excluding them doesn't noticeably speed up typecheck updates (because nearly all the time is spent compiling a single gem,parser
, for reasons i haven't yet debugged), so this may be viewed as an unnecessary optimization (though it does reduce the lines of code checked in by >10%). This wasn't an exhaustive search, so folks may find more gems that could be added to the list.