-
Notifications
You must be signed in to change notification settings - Fork 154
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
Allow for indexing enhancements through included hooks #2358
Conversation
Fantastic bit of technical writing as always. Excited to see this go.
I think that one of the latter two choices may be better depending on the technique used. If customizations are modifying the indexing techniques, then I think EDIT: |
Another possible name - supplement. Here's what GPT says: The terms "supplement" and "augmentation" both refer to the process of adding something, but they are used in slightly different contexts and have nuanced meanings. Supplement
Augmentation
Key Differences
In summary, while both terms involve adding something, "supplement" focuses on completion and enhancement, whereas "augmentation" focuses on increasing and amplifying. |
I'll test the API with the RSpec addon too. ruby-lsp-rspec-definition-demo.mp4 |
c83da98
to
0a694ab
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.
For naming, it would be great to have a more approachable name. I think supplement
is better than augmentation
, but it's a bit rare to see it in software context too (from my limited experience). So enhancement
will be my favorite.
0a694ab
to
de59d8d
Compare
Switched the name to enhancements. |
c6ac782
to
a7a0a1b
Compare
I added basic error reporting around enhancement so when it encounters an error, the file's indexing can still be completed and we can get some messages for debugging, like:
|
This change rescues errors raised when indexing with enhancements and logs them to stderr when the file's indexing is finished.
a7a0a1b
to
5269522
Compare
Motivation
This PR is the first step for allowing addons to enhance the indexing mechanism and a proposal of how we could handle included hooks.
Example usages Shopify/ruby-lsp-rails#422, Shopify/ruby-lsp-rails#423
What does a concern do?
For context, in order to make a module into a concern one has to
extend ActiveSupport::Concern
. Upon extending, the concern module will then dynamically define anincluded
hook into the module that has extended it. Finally, the included hook will dynamically extend anyClassMethods
module that exists inside of the original module.So we need a way to
Other use cases
I have also considered other use cases, like creating a new instance method during an
extend
or handling something likebelongs_to
(which creates new methods on the receiver).Naming
I ended up going with "indexing augmentations" for the name of concept. The idea is to avoid using extension and plugin (too associated with editors) or addon (now associated with the language server).
Do we agree with the naming? Is indexing enhancement better? Indexing contribution?
Implementation
The implementation has a few parts:
Notes
I believe we do not have to support lazy extended, prepended or inherited hooks. The reason is because the concern pattern is broadly used to mix instance and class methods in a single operation. You include one module and then you get everything you need.
The opposite pattern of extending a module and then getting a dynamic include is not really common and would essentially just be the exact same thing as a concern.
And inheriting from a class already carries both instance and class methods, so there's also reduced need for meta-programming on that side.
We may discover other scenarios later, but I think it should be fine to handle included hooks only for now.
Automated Tests
Added tests.
Manual Tests
There's no way to test this without the changes being made to the Rails addon too. I did it locally and recorded it in action in a toy app.
augmentation_demo.mov