- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 162
          Add solargraph profile command using vernier
          #1071
        
          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
base: master
Are you sure you want to change the base?
Conversation
a2695f0    to
    872431c      
    Compare
  
    | I'm not sure how my changes could lead to such breaking of the typechecking step here. @apiology do you recognise anything obvious there by any chance? | 
| directory = File.realpath(options[:directory]) | ||
| FileUtils.mkdir_p(options[:output_dir]) | ||
|  | ||
| host = Solargraph::LanguageServer::Host.new | 
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.
I have based the host setup, based on this spec:
| it 'finds definitions of methods' do | 
Is there any critical step missing that should be included in the profile?
| def host.send_notification method, params | ||
| puts "Notification: #{method} - #{params}" | ||
| end | 
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.
I thought I'd never find a use-case for this Ruby feature, but I finally found one 😍
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.
Hey! This is the line that triggered that assert - looks like Solargraph has never run through that code in its tests or typechecking in CI, either!
https://github.com/apiology/solargraph/blob/master/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb creates some pins, but doesn't pass the 'source' kwarg in. If you throw "source: :parser" into that "Solargraph::Pin::Namespace.new" call, it should get past that error.
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.
Thanks again, @apiology, for coming to the rescue! 🙇♂️
For some reason, I thought the typechecker does not cover the shell area.
| 
 I've been thinking about doing the same thing as a functionality test. @AaronC81 has some interesting work here in Sord that we could probably import with credit: https://github.com/AaronC81/sord/blob/master/Rakefile#L121-L138 | 
        
          
                solargraph.gemspec
              
                Outdated
          
        
      | s.add_development_dependency 'undercover', '~> 0.7' | ||
| s.add_development_dependency 'overcommit', '~> 0.68.0' | ||
| s.add_development_dependency 'webmock', '~> 3.6' | ||
| s.add_development_dependency 'vernier' | 
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.
| s.add_development_dependency 'vernier' | |
| s.add_development_dependency 'vernier', '~> 1.8' | 
This will keep a 2.0 release from breaking our build so we can update on our own timeline, and make sure weird things don't happen if a user has an old version installed.
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.
| My main concern with integrating Vernier into the codebase is that it doesn't compile on Windows. A similar issue with EventMachine used to cause a lot of headaches. | 
| 
 @castwide  I was not aware of this bthw - the main reason why I kept it as an optional dependency is to not include unnecessary dependencies to end-users. If someone needs to profile it with the latest version, they can install it manually (see  We can always remove it as a development dependency, too, and keep it as an optional dependency in both contexts. | 
| 
 Happy to hear that, we're seeing benefits out of this already 🙌. I look forward to having this as a CI step, where we can formally track performance changes. Thanks for the suggestion here, that looks like a good start! | 
6be19ba    to
    a13322a      
    Compare
  
    | @apiology, sorry to harass you again, but do you know why there are discrepancies between overcommit ❌ and typechecking ✅ step? Are they using different levels of typechecking? Or a different Ruby/rbs version, maybe? UPDATE: I ran it locally ( I fixed them here, so let's see.. UPDATE: One difference that I see between these CI steps, is caching: solargraph/.github/workflows/linting.yml Lines 42 to 52 in 453a6e9 
 vs 
 Could that have a role? | 
5af206e    to
    453a6e9      
    Compare
  
    | 
 Will it work if we leave it out altogether, add a require inside the function within a try / rescue block, and tell people to install the gem themselves? We could also mark the command as hidden (like my PR for the pin command) | 
| 
 Yep! The overcommit uses the strong level for the bits you change, the rest is done at 'typed' level. | 
| The errors you're seeing at strong level in overcommit will be fixed in #1059 - you can either merge that branch in or just ignore the errors entirely. | 
| 
 We already do that except for development. 
 So I question how worth it that is here. I'd prefer more people know about this command so they can report issues they notice on their side. | 
453a6e9    to
    431dd32      
    Compare
  
    
Hi,
I thought of adding a profiling command for convenience, to avoid the hassle of configuring the IDE, switching gem versions, restarting, and all that:
Example Output
By default, the
solargraph profilewill run against the current directory, and will find the first Ruby file with the default line/number.When testing new local changes, one could use the
bundle exec solargraph --directory=/path/to/a/real-project ..other paramsHere's a quick demo: here
I hope you find it useful.
Future Work
Maybe as a next step, we could select some large open-source Rails projects that we can use to profile and track the merged changes in main through a CI step. Any suggestions for such projects? Personally, I've used https://github.com/gitlabhq/gitlabhq in numerous cases, but https://github.com/mastodon/mastodon also come to mind as good candidates. WDYT?