Testing @grncdr's hacks #43
Conversation
|
@iftheshoefritz please do a pull from master, let's see what CI says |
|
Hey sorry about the lack of communication, I've been pretty busy lately and I wanted to get some more testing of this branch done before opening a PR, but I really haven't had the time. About the CI failures, I haven't looked to closely but I suspect they won't be too hard to fix. There's probably conflicts between the Easy stuffEverything up to and including 6989d59 (compare), (plus 0338071 which fixes a regression) is more or less harmless. Blocked on solargraph changes0f9e4af depends on castwide/solargraph#602 which hasn't seen a lot of movement. I get the impression @castwide is also very busy, so if anybody has ideas on how to streamline that PR I'm all ears. The bigger stuffd6a9b12 is the main change, and it's a pretty radical departure from the approach taken in this gem so far. The commit message explains the what/why in more detail, but to summarize: YARD types and the Solargraph type-checker are not capable of expressing ActiveRecord APIs in a truly generic fashion. Generating a load of types/pins at runtime gives a far better result, and honestly isn't that much more work. I'm pretty sure that this contradicts the approach being taken with |
|
@grncdr I agree with your assessment, thanks for explaining so clearly in that commit. I'm all for continuing as you have, let's move away from trying to make YARD do things it can't. |
solargraph-rails.gemspec
Outdated
| @@ -1,4 +1,4 @@ | |||
| lib = File.expand_path('../lib', __FILE__) | |||
| lib = File.expand_path('lib', __dir__) | |||
There was a problem hiding this comment.
this is interesting, what is the importance of the difference here? (not pushing back, just curious)
There was a problem hiding this comment.
Hm, I think that's an autofix by Rubocop.
grncdr
force-pushed
the
hacks
branch
2 times, most recently
from
4737228
to
9bb17ee
Compare
Doing it this way ensures that generated YARD docs are present before the setup-ruby action updates it's cache.
These contained a load of duplicated definitions for methods that are inherited from Module. This will make tests a teensy tiny bit faster, but mostly makes these files actually sort of useful. These changes were made manually, the generate_definitions.rb script should probably be updated to be a bit more precise, but we can deal with that the next time somebody feels like running it.
If another test instantiates the Annotate class with a schema is present first, this test would fail. Singletons ...
This (as far as I know) matches what Rails will do when it doesn't understand a database type.
This a more accurate represenation of what ActiveRecord actually does at runtime, and therefore gives better code suggestions when dealing with associations and model classes. I've settled on this as being the best compromise after trying a few different approaches. There are two "challenges" that I believe can't be met any other way (at this time). 1. It is not possible to write a return type annotation for the methods of various ActiveRecord mixins that will be correct for both a model class (e.g. Person.where) and relation (e.g. people.where). [0], [1] 2. It is not possible to represent ActiveRecords "class methods are also relation methods" behaviour without model-specific relation types. It's conceivable that Solargraph could change it's interpretation of [self] on class methods to solve the first challenge, but the second one really forces our hand. In order to represent this correctly, Solargraph would need support for method-missing delegation, **and** delegating those missing methods to an associated/generic type. Given @castwide is working on RBS support and the Ruby ecosystem is likely to move that way in the future, it seems pragmatic to eat the cost repetition / manual labour in this gem rather than try to push YARD types into supporting that degree of type-level programming. 😅 [0]: castwide/solargraph#592 [1]: lsegal/yard#1257
Setting this binding the models hidden relation type makes it possible to refer to other scopes and the ActiveRecord API with correct types when defining a scope.
This fixes errors when the typecheck reporter is enabled.
The binder is expected to be a Solargraph::ComplexType
This _should_ be easier to both maintain and contribute to.
|
@grncdr over in #59 I can see that although This looks like it might just be the solargraph-rails dependencies that are installed there. |
No description provided.