Add Rspec included modules support #13
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,3 +11,7 @@ | |
|
|
||
| # rspec failure tracking | ||
| .rspec_status | ||
|
|
||
| .solargraph.yml | ||
| vendor | ||
| gemfiles/vendor | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| --- | ||
| BUNDLE_RETRY: "1" | ||
| BUNDLE_PATH: "vendor/bundle" |
|
Owner
There was a problem hiding this comment. important: These changes are more appropriate for the Additionally, this approach helps avoid redundant requires and pins in individual spec files, which could unnecessarily increase Solargraph's memory usage.
Contributor
Author
There was a problem hiding this comment. True - the one concern I have (which btw, not done right now) is "how do we handle updates to this file"? Imo the simple and easy approach is to call reset if the file name matches one of the configured paths but idk how I'd propagate that to the rest of the files (even via global)
Contributor
Author
There was a problem hiding this comment. Imo its not a HUGE issue since those files are edited rarely (let alone to include a module) but still, something to think about
Owner
There was a problem hiding this comment.
That's a very good point that I overlooked. As you said, these files tend not to change often, and I also don't find that to be the end of the world. One in theory could fix that by restarting the LSP, albeit not an ideal solution. What we could do, though, is to include these pins in I think/hope this would ensure that whenever these files are changed, we would get the latest includes on the spec files. Can we try this? If it does not work, I'd still go with |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Solargraph | ||
| module Rspec | ||
| # RSpec.configure ... config.include handler, essentially | ||
| class SpecHelperInclude | ||
|
ShadiestGoat marked this conversation as resolved.
Outdated
|
||
| COMMON_HELPER_FILES = [ | ||
| 'spec/spec_helper.rb', | ||
| 'spec/rails_helper.rb' | ||
| ].freeze | ||
|
Comment on lines
+7
to
+
Owner
There was a problem hiding this comment. Suggestion: Make these configurable and move to |
||
|
|
||
| # @param node [::Parser::AST::Node] | ||
| # @param file [String] The name of the file this is module is defined in | ||
| # @param module_name [String] The name of the module to be included | ||
| INCLUDED_MODULE_DATA = Struct.new(:node, :file, :module_name) | ||
|
ShadiestGoat marked this conversation as resolved.
Outdated
|
||
|
|
||
| def self.instance | ||
| @instance ||= new | ||
| end | ||
|
|
||
| def self.reset | ||
| @instance = nil | ||
| end | ||
|
|
||
| # @return [Array<Solargraph::Pin::Reference::Include>] | ||
| def pins | ||
| ns = Solargraph::Pin::Namespace.new(name: 'RSpec::ExampleGroups') | ||
|
Owner
There was a problem hiding this comment. Please re-use |
||
| ns2 = Solargraph::Pin::Namespace.new(name: 'RSpec::Example') | ||
|
ShadiestGoat marked this conversation as resolved.
Outdated
|
||
|
|
||
| included_modules.flat_map do |m| | ||
| [ | ||
| Solargraph::Pin::Reference::Include.new( | ||
| closure: ns, | ||
| name: m.module_name, | ||
| location: Solargraph::Location.new(m.file, Solargraph::Parser.node_range(m.node)) | ||
| ), | ||
| Solargraph::Pin::Reference::Include.new( | ||
| closure: ns2, | ||
| name: m.module_name, | ||
| location: Solargraph::Location.new(m.file, Solargraph::Parser.node_range(m.node)) | ||
| ) | ||
| ] | ||
| end | ||
| end | ||
|
|
||
| def extra_requires | ||
| included_modules.map(&:file).uniq + Dir['spec/support/**/*.rb'] | ||
|
Owner
There was a problem hiding this comment.
If we do not exclude the |
||
| end | ||
|
|
||
| # @return [Array<INCLUDED_MODULE_DATA>] | ||
| def included_modules | ||
| @included_modules ||= parse_included_modules | ||
| end | ||
|
|
||
| private | ||
|
|
||
| # @return [Array<INCLUDED_MODULE_DATA>] | ||
| def parse_included_modules | ||
| modules = [] | ||
|
|
||
| COMMON_HELPER_FILES.each do |f| | ||
| ast = Solargraph::Parser.parse(File.read(f), f) | ||
| modules += extract_included_modules(ast, f) | ||
| rescue Errno::ENOENT | ||
| # Ignore this error - no file means we can chill | ||
| rescue StandardError => e | ||
| Solargraph.logger.error("[solargraph-rspec] [spec helper] Can't read helper file '#{f}': #{e}") | ||
| end | ||
|
|
||
| modules | ||
| end | ||
|
|
||
| # Parses the modules that were included int he Rspec.configure (in common helper files) | ||
| # @param ast [Parser::AST::Node] | ||
| # @param file [String] | ||
| # | ||
| # @return [Array<INCLUDED_MODULE_DATA>] | ||
| def extract_included_modules(ast, file) | ||
|
Owner
There was a problem hiding this comment. Can we please extract this into a: I would prefer to keep everything isolated within that module, while keeping the plugin/pin logic in this class. That has proven helpful when switching between different parsers last time.
Contributor
Author
There was a problem hiding this comment. What do you think about having a spec ast module? It'll include the normal walker, the spec walker & the new config walker?
Owner
There was a problem hiding this comment. I'd prefer keeping unit tests matching the file names and separated. It makes it easier to navigate between source/test files (I use https://github.com/tpope/vim-projectionist) or run them via guard (which I plan to add in the future). For integration-like specs (ie. test cases in
Contributor
Author
There was a problem hiding this comment. Hey sorry - I think I worded another thing badly. I meant a module for walkers in general, which would include the spec walker that already exists & a new class for the rspec config walker (and, in the future, a factory bot walker)
Owner
There was a problem hiding this comment. Oh yeah, that would make totally sense! 💯 |
||
| walker = Walker.new(ast) | ||
|
|
||
| # @type [Array<INCLUDED_MODULE_DATA>] | ||
| included_modules = [] | ||
|
|
||
| walker.on :block, [:send] do |node| | ||
| send_node = node.children[0] | ||
| send_receiver = send_node.children[0] | ||
|
|
||
| next if send_receiver.type != :const || send_receiver.children[2] == :Rspec | ||
| next unless send_node.children[1] == :configure | ||
| # No args | ||
| next if node.children[1].children.empty? | ||
|
|
||
| config_name = node.children[1].children[0].children[0] | ||
| config_walker = Walker.new(node) | ||
| config_walker.on :send, [:lvar, config_name] do |include_node| | ||
| next unless include_node.children[1] == :include | ||
|
|
||
| mod_node = include_node.children[2] | ||
| next unless mod_node.is_a? ::Parser::AST::Node | ||
| next unless mod_node.type == :const | ||
|
|
||
| included_modules << INCLUDED_MODULE_DATA.new( | ||
| include_node, file, SpecWalker::FullConstantName.from_ast(mod_node) | ||
| ) | ||
| end | ||
|
|
||
| config_walker.walk | ||
| end | ||
|
|
||
| walker.walk | ||
|
|
||
| included_modules | ||
| end | ||
| end | ||
| end | ||
| 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.
Good catch, but I think we need to check
solargraph-rspec.gemspec, andsolargraph-rspec/Gemfile, andAppraisalsfile instead.The
solargraph-rspec/gemfiles/default.gemfile.lockis just a product of theAppraisalsfile, where in the future there could potentially be more dependency versions.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'm actually not so sure - the gemlock file is the source of truth on pinned gems for tests, which could be updated without either of those 2 changing