Integrate document symbol for callbacks #280
Conversation
| return unless node.arguments | ||
|
|
There was a problem hiding this comment.
This is not in the direct scope of this PR, but I've kept it to a separate commit; it is used by the DocumentSymbol class. The line below already accomplishments what this does.
aryan-soni
force-pushed
the
as/document-symbol-callbacks
branch
2 times, most recently
from
f5e0bf4 to
9146188
Compare
| MODEL_CALLBACKS = T.let( | ||
| [ | ||
| "before_validation", | ||
| "after_validation", | ||
| "before_save", | ||
| "around_save", | ||
| "before_create", | ||
| "around_create", | ||
| "after_create", | ||
| "after_save", | ||
| "after_commit", | ||
| "after_rollback", | ||
| "before_update", | ||
| "around_update", | ||
| "after_update", | ||
| "before_destroy", | ||
| "around_destroy", | ||
| "after_destroy", | ||
| "after_initialize", | ||
| "after_find", | ||
| "after_touch", | ||
| ].freeze, | ||
| T::Array[String], | ||
| ) | ||
|
|
||
| CONTROLLER_CALLBACKS = T.let( | ||
| [ | ||
| "after_action", | ||
| "append_after_action", | ||
| "append_around_action", | ||
| "append_before_action", | ||
| "around_action", | ||
| "before_action", | ||
| "prepend_after_action", | ||
| "prepend_around_action", | ||
| "prepend_before_action", | ||
| "skip_after_action", | ||
| "skip_around_action", | ||
| "skip_before_action", | ||
| ].freeze, | ||
| T::Array[String], | ||
| ) | ||
|
|
||
| JOB_CALLBACKS = T.let( | ||
| [ | ||
| "after_enqueue", | ||
| "after_perform", | ||
| "around_enqueue", | ||
| "around_perform", | ||
| "before_enqueue", | ||
| "before_perform", | ||
| ].freeze, | ||
| T::Array[String], | ||
| ) | ||
|
|
There was a problem hiding this comment.
I've linked the source for each of these in the description, but the list is intended to be exhaustive.
Since callbacks remain relatively constant over time, this shouldn't be that suboptimal.
There was a problem hiding this comment.
🤔 Side thought, no need to change, but I wonder if we could get the callbacks list programmatically from Rails at runtime...
There was a problem hiding this comment.
Yeah, I wasn't too happy with this approach but was unable to find a list available with these.
| selection_range: range_from_node(node), | ||
| range: range_from_node(node), | ||
| name: name, | ||
| kind: RubyLsp::Constant::SymbolKind::METHOD, |
There was a problem hiding this comment.
This affects the icon of the symbol.
I chose the same approach as the base Ruby LSP. The options are listed in this link — I don't think it's optimal to use any other config.
|
Not a blocker, but it could be useful to add some callbacks to the dummy app, to allow for easy manual verification. |
|
Again not a blocker, but I notice it doesn't handle this syntax: before_create ->(user) { user.name = user.login.capitalize if user.name.blank? }(from the docs) |
|
I assume we've done some testing on Core with various syntaxes? |
The dummy app as in the It was initially my understanding that it should be kept lean. |
Good catch, will add in support. Looks like we'll also have to handle a |
Yeah it doesn't need to exhaustive, just enough to verify that we haven't broken it badly. |
There was a problem hiding this comment.
Can we try splitting this into 2 classes to make things easier to understand in the future? Like DslDocumentSymbol and TestDocumentSymbol.
It should be doable and fairly simple under the new addon system. If not, we can also use the feedback from this attempt to improve it.
I do agree that we have to create the abstraction at some point, but I'd rather scope it out as this PR already has a lot of points for discussion. After @andyw8's review, I noticed we should also add support for classes, like I think it would be best to extract the classes out after I get to working on the other 2 dsl features. |
aryan-soni
force-pushed
the
as/document-symbol-callbacks
branch
6 times, most recently
from
4ccf59d to
012bf75
Compare
Since other features are all dsl related, wouldn't this make the class separation more beneficial as the reviews will be easier? |
My thinking is that it'll be easier to figure out what behaviour we can unify once we examine the behaviour of the other classes. In the dsl class, I don't think it'll be as simple as one method that applies to all of the features, so we can make decisions on the abstraction then. I could separate things once I get to the next feature, instead of waiting until the very end. I agree that would make future reviews cleaner. |
aryan-soni
force-pushed
the
as/document-symbol-callbacks
branch
from
012bf75 to
a5543ed
Compare
Callbacks are ubiquitous across Rails, so it would be helpful to explicitly isolate them in the LSP via document symbols.
This approach accounts for callbacks associated with
ApplicationRecord(models),ApplicationController(controllers), andApplicationJob(jobs).In addition to the various callback categories, there are 3 types of arguments to consider:
before_perform :foo_method).before_save "foo_method").before_action do ... end).So, this implementation looks like this:
As per this approach, something like
before_save "foo_method", "bar_method", on: :updatewould yield two document symbols:before_save(foo_method)andbefore_save(bar_method).This approach directly ties to what is supported in Ruby LSP already for something like
attr_reader :foo.See my thoughts below for additional context on the PR.