Skip to content
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

Help understanding limitations with Rails and Solargraph #188

Closed
thornomad opened this issue Jun 3, 2019 · 7 comments
Closed

Help understanding limitations with Rails and Solargraph #188

thornomad opened this issue Jun 3, 2019 · 7 comments

Comments

@thornomad
Copy link

Very new to solargraph so forgive me is this is obvious. I see from #87 that Rails support is still in the pipeline -- but as I am new I want to make sure I am covering my bases.

Is there a way to get instances of a class recognized as belonging to the class they are coming from with methods such as MyActiveRecord.find()?

For example: Person.find(1) (no) vs Person.my_find() (yes)

# @!attribute my_attribute
#   @return [String] example attribute
class Person < ApplicationRecord
  
  # Custom find method that works as expected
  # 
  # @return [Person]
  def self.my_find(id)
    Person.find(id)
  end

end

Correctly yard doc'd custom methods work:

image

Rails built-in does not

image

Is this a problem because Rails is not using YARD formatting for their "find" method? Or with find being inherited ? Or with my setup?

include:
- "**/*.rb"
exclude:
- spec/**/*
- test/**/*
- vendor/**/*
- ".bundle/**/*"
require: []
domains: []
reporters:
- rubocop
- require_not_found
plugins:
- actioncable
- actionmailer
- actionpack
- actionview
- activejob
- activemodel
- activerecord
- activestorage
- activesupport
require_paths: []
max_files: 5000

Thanks in advance for any insight.

@castwide
Copy link
Owner

castwide commented Jun 4, 2019

One issue with your .solargraph.yml: the paths in the plugins section should be in require. That might get you a little more intellisense, but it still won't be complete.

Solargraph doesn't know that Person.find should return a Person because Rails doesn't provide a @return tag for the find method.

One workaround is to identify the variable type with a @type tag:

# @type [Person]
person = Person.find(100)
person.my_a # <- works

@thornomad
Copy link
Author

thornomad commented Jun 4, 2019

Thank you for your reply. I've fixed the .solargraph.yml issue. What you describe about Rails not providing the documentation is disappointing—but what I was suspecting. I wasn't sure if there was even a way to have a child class dynamically assign its return values based on documentation from the parent class.

I'm curious how other users of Rails and Solargraph deal with this issue?

My imagined workaround at this point is to create a module (Solargrapher perhaps or Documenter) and then add it to each of my ActiveRecord models that provide references to the parent instance and class methods such as:

# @return [Person]
def self.find(id)
  super(id)
end

# @return [Class<Person>]
def self.where(*args, **options)
  super(*args, **options)
end

# etc ...

Kind of a hassle but not a lot of work -- copy paste and find replace all. But I wonder if there is a better way to get this functionality? The real hassle would be adding a new super method after already creating all the files.

@castwide
Copy link
Owner

castwide commented Jun 4, 2019

One thing that could make it a little cleaner is using YARD directives to generate the documentation without modifying the actual code. There are currently a couple bugs in the way directives get parsed (see #176) that I plan to have fixed in gem 0.33.0, but there's a workaround for now.

You can use the @!parse directive to add "virtual" code to the documentation like this:

# @!parse
#   class Person
#     def self.find(*args)
#       Person.new
#     end
#   end

The problem with @!parse is that you can't embed YARD tags inside it, so you have to specify Person.new in the method for Solargraph to infer the return type.

In version 0.33.0, you'll also be able to do it with the @!method directive:

class Person
  class << self
    # @!method find(*args)
    #   @return [Person]
  end
end

There's an even better way that's in the works. It would make the most sense to use the @!method directive in ActiveRecord::Base itself:

class ActiveRecord::Base
  class << self
    # @!method find(*args)
    #   @return [self]
  end
end

That way, any class that extends ActiveRecord::Base will automatically inherit the virtual find method. Unfortunately, there's a bug in the way that maps resolve self in @return tags. I hope to have it fixed in 0.33.0, but it might need to wait for a patch revision.

The YARD tags overview might give you some other ideas.

@thornomad
Copy link
Author

Thanks for the insight again -- looking forward to 0.33. I agree that the last suggestion is the most more better! Can't wait.

@thornomad
Copy link
Author

thornomad commented Jun 18, 2019

There's an even better way that's in the works. It would make the most sense to use the @!method directive in ActiveRecord::Base itself

It looks like this is working now in 0.33 ! Woot! Thank you!

Quick follow up question ... is this the correct way to do it for where and find ? (i.e., [Class<self>])

  # @!method self.where(*args, **options)
  #   @return [Class<self>]

  # @!method self.find(*args, **options)
  #   @return [self]

PS:

I noticed when I used class << self approach ... the second and subsequent @!method definitions did not get picked up. When I switched to self.find etc it works.

Thank you thank you thank you!

@castwide
Copy link
Owner

Glad you got it working!

I've been using @return [self] for find in my tests. It doesn't cover every possible case, since certain parameters will make it return Array<self> instead, but it's a start.

For where, I think @return [ActiveRecord::Relation] is more technically correct, but you might lose some context that way.

I encountered the same bug with multiple @!method directives inside class << self. I'll try to get it fixed in a patch release.

@thornomad
Copy link
Author

For where, I think @return [ActiveRecord::Relation] is more technically correct, but you might lose some context that way.

Yea - I kept going back and forth between those two options. Class methods on the model are not available using ActiveRecod::Relation though—which is often what I need the most help with. I thought maybe I could do [ActiveRecord::Relation<MyModel>] or something to indicate it was a child of it ... but no luck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants