Conversation
Noticed when testing in solargraph-rspec TODO: add clip_spec.
Avoid cloning the hash
Use the base method defaulting to undefined complex type
Comes from the new solargraph-rspec specs related to this PR
| method_call_chain = NodeChainer.chain(send_node, @filename, nil, []) | ||
| literal = Chain::Literal.new(lit, n) | ||
| arg_index = send_node.children[2..-1].index(n) | ||
| result.push Chain::Parameter.new(literal, method_call_chain) |
There was a problem hiding this comment.
Initially, I wanted to make this work as a chain of: method_call > parameter; however, I struggled to find a good way to pass the last method call resolution to the Chain::Parameter as a context here:
solargraph/lib/solargraph/source/chain.rb
Lines 108 to 133 in 48f3d08
So I ended up with parameter(method_call_chain).
Maybe I don't understand the Pin::ProxyType well enough, so if you have a suggestion on how to make it work, I'd be more than happy to refactor this into an "appropriate chain".
|
|
||
| module Solargraph | ||
| module Pin | ||
| class FactoryParameter < Base |
There was a problem hiding this comment.
Open to other naming suggestions!
| map = Solargraph::SourceMap.map(source) | ||
| cursor = map.cursor_at(Solargraph::Position.new(2, 10)) # :bar | ||
| expect(cursor.chain.links.last).to be_a(Solargraph::Source::Chain::Parameter) | ||
| expect(cursor.chain.links.map(&:word)).to eq(['foo(.., <::Symbol>, ..)']) |
There was a problem hiding this comment.
I'm NOT very happy with this "word notation": foo(.., <::Symbol>, ..), other ideas?
Another one that I thought of was: foo(name=<::Symbol>), maybe?
aad1bf2 to
c82fd8c
Compare
c82fd8c to
9610eb0
Compare
|
Hi - can you explain what you mean by "factory like"? I don't think I get it |
Good question @ShadiestGoat! By factory-like methods I mean the methods which build other objects (classical definition), a typical example being And in Ruby, this extends to DSL class methods that "build the class", by adding methods (eg, Rails model associations) or customising its behaviour (eg, Rails validations). This was the initial idea behind the Does that make sense? |
Yes thank you for clarifying. One more question - I don't think I understood how to actually use them? In the case of factory bot for eg? What do the various params do? |
I actually use the FactoryBot case to describe it in Also see FactoryParameter#initialize method documentation. Let me know if the spec is still unclear. |
|
Oh that's great - I have not had a chance to look at any code, so I didn't catch that. I was just going of off the PR description. As a side note, maybe it'd be good to link to it in the description, for silly people like me? |
|
Very interesting. I wonder how much better this is than a method override as I proposed in my factory bot pr? Also, I might've just missed this in my quick glance, but is this able to determine other parts of the signature based on the value? Like other params available? Obviously I'm no maintainer here and I haven't touched this codebase in like at least a month so I have no say, but given examples from other languages like typescript, it makes more sense to me to do overrides w/ different signatures than a different type of pin. Like imo the signature detection and use of it should improve rather than adding smt like this. What do you think? Like the fundamental problem is that solargraph's completion & validation for constants/literals is lacking & that override go to definition is not supported. Those are the problems that need to be fixed. Especially since this is currently valid code according to sg (afaik): # @param arg ['foo']
def example(arg)
end
example('bar') |
|
Thank you so much @ShadiestGoat for your feedback - you’ve raised some excellent questions! On the question of “why introduce a new pin instead of using overload signatures?”: I see overloads as most useful when a method has a finite set of known input values (for example, enum-like parameters such as As far as I know, no type system can fully capture this sort of dynamic binding. That’s why I believe it’s more appropriate for a solargraph plugin to provide that "conventional connection" via a new
That was outside the scope of this particular PR (it wasn’t needed for my case), but the current implementation could certainly be extended to support something along those lines. As for:
I agree with you here - but I see that as a separate issue, more aligned with the case I mentioned earlier where values are known ahead of time from the signature itself. I’m not dismissing your suggestion at all; in fact, I’d love to hear what @castwide thinks as well. I just wanted to clarify my reasoning for introducing a new pin instead of relying on overloads, as you did in your FactoryBot PR (which I really appreciate ❤️). And please don’t take this PR as a blocker for your work there! TL;DR: Using overloads here felt at least semantically incorrect, and on top of that, they seemed harder to make work (at least per my current understanding of the codebase). |
|
From the practical perspective, another reason why I favoured the new pin over overload is that with the later: RSpec.include_examples("some shared example")
# ^^^^^^^^^^^^ Uses needs to click on the method name to jump to the definition
# We lose the method definitionWhereas with the new pin RSpec.include_examples("some shared example")
# ^^^^^^^^^^^^^^^^^^^ User clicks on the param
# ^^^^^^^^^^^^ User can still jump to "real method definition" |
|
That's fair - thank you for the expansion. I do agree that it's not exactly ergonomic to use overloads. Your last point sounds like a really cool use case (though I'm not 100% convinced that it can't be done with overloads). I'm not sure I understand about your first point - rspec shared examples, factory models, or even rails PS: Just like you, I'm also not suggesting that this is a bad implementation/idea, I'm trying to explore this idea/issue further. I really love the rspec plugin and it has saved me tons of headaches at work ❤️ |
You're actually right here, it's rather a question of should we, and later how hard and efficiently we could do that.
I meant from the perspective of the method definition. I think, Let me use That is why I think it makes more sense to have a separate construct for this purpose. (Not 100% sure though!) Maybe I'm being over-pedantic here.
This really warms my heart to hear! 🥰 |
Relates to: lekemula/solargraph-rspec#18 and lekemula/solargraph-rspec#14
Problem
I would like to add support for RSpec shared examples via solargraph-rspec plugin: solargraph-rspec#18
Current limitation: When using RSpec shared examples like this:
Solargraph cannot:
"some example"Solution
This PR introduces a new
Pin::FactoryParameterclass that enables plugins (particularly solargraph-rspec) to register method parameters with specific values and their corresponding definitions.How it works
The new pin allows plugins to define factory-like method parameters:
See FactoryParameter#initialize method documentation what each parameter means.
Key benefits
Why a new pin type?
This approach creates a flexible foundation that can support multiple Ruby DSL patterns beyond RSpec:
Immediate applications
include_examples "example_name"include_context "context_name"Future possibilities
create(:model_name)with go-to-definition to factory definitionsbelongs_to :model_namewith navigation to model filesvalidates :method_namewith navigation to method definitionsScope of this PR
This PR focuses on implementing the core infrastructure and two primary features:
✅ Go-to-definition support - Navigate from parameter values to their definitions
✅ Return type inference - Lay groundwork for better type resolution
These features directly address the most requested functionality from the solargraph-rspec.
Future enhancements
Solargraph::Source::Chain::ParameterinfrastructureQuestions for review
Pin::FactoryParameteran appropriate name and structure?