Stop monkey-patching core classes #481
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This causes a confusion with the `Class#descendants` method that Active Support adds and the signature on this method leaks out into the Tapioca generated RBI and causes type-checking problems in Core because of it. This is arguably a reflection method, so I moved it to `Tapioca::Reflection` module which is already included in the Base DSL class and SymbolGenerator class.
This causes a confusion with the `String#underscore` method that Active Support adds and causes problems like https://discourse.shopify.io/t/file-naming-inflection-issues-with-tapioca-dsl-verify-in-ci/19663 I moved the method onto the `Generator` class and also duplicated it into the DSL test base class.
paracycle
force-pushed
the
uk-fix-patching-problems
branch
from
a515102
to
ffa085b
Compare
mutecipher
reviewed
Sep 3, 2021
| underscore(target_class_name) | ||
| end | ||
|
|
||
| sig { params(class_name: String).returns(String) } |
There was a problem hiding this comment.
This seems to be replication of the previous declaration of this method in lib/tapioca/generator.rb. Would it be better to make this a helper module somewhere we can inject instead?
There was a problem hiding this comment.
Indeed it is, but since this is a test base class, I wasn't too worried about refactoring to a helper class/module. The real usage is in generator.rb only.
RyanBrushett
approved these changes
Sep 3, 2021
vinistock
approved these changes
Sep 7, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Monkey patching
Classto adddescendantscauses a confusion with theClass#descendantsmethod that Active Support adds and the signature on this method leaks out into the Tapioca generated RBI which causes type-checking problems in Core.Monkey patching
Stringto addunderscorecauses a confusion with theString#underscoremethod that Active Support adds and causes problems like https://discourse.shopify.io/t/file-naming-inflection-issues-with-tapioca-dsl-verify-in-ci/19663 (since the method later on gets replaced by the AS version and starts using application inflectors, etc, which we don't want, we just want a simple deterministic CamelCase to under_score implementation)Implementation
descendantsis arguably a reflection method, so I moved it toTapioca::Reflectionmodule, which is already included in the Base DSL class and SymbolGenerator class, asdescendants_of(klass). The added benefit is that through type parameters, we get better typing on this method, thus we need less casts.I moved the
underscoremethod onto theGeneratorclass and also duplicated it into the DSL test base class.Tests
No added tests.