Upgrade to ruby-lsp 0.26 - #6
Merged
Merged
Conversation
Bump the ruby-lsp dependency from `~> 0.17` to `~> 0.26` and raise required_ruby_version to >= 3.0.0 to match what ruby-lsp now requires. Addon changes: - Implement `version`. Since 0.23 the addon base class treats it as abstract and uses it for the `Addon.get(name, constraint)` compatibility check, so without it any addon querying this one raises AbstractMethodInvokedError. - Drop the sorbet-runtime dependency. ruby-lsp no longer depends on it and the only reference left here was a bare require; there is no `T.` or `T::Sig` usage anywhere in the gem. Test changes: the suite hand-rolled `with_server` against internals that have since been removed (`core_ext/uri`, `RubyIndexer::IndexablePath`). It now uses the `RubyLsp::TestHelper` module that ruby-lsp ships for addons, requires `ruby_lsp/internal` instead of a long explicit require list, and uses `pop_result` so notifications in the queue don't break the assertions. The expected reek docs URL is interpolated from `Reek::Version::STRING` rather than hardcoded. Clean up the reek warnings so `rake code_analysis` passes: - `Runner#build_examiner` (FeatureEnvy) is gone. It existed to work around Examiner not accepting a separate source and origin, reaching into two of its instance variables to do so. `SourceCode.from(source, origin:)` takes an explicit origin and Examiner passes a SourceCode through untouched, so the workaround is no longer needed. - `Runner#warning_to_diagnostic` (UtilityFunction) moves to `Reek::Diagnostic.from_warning`, which is what the smell was suggesting. - `Runner#run_formatting` (UtilityFunction) is suppressed in place. The Formatter interface requires an instance method and reek is a linter, so there is nothing to format and no instance state to depend on. The origin behaviour from #5 had no test, so add one covering it: a file under a directory with a `.reek.yml` directory directive is exempt while a file outside it is not. Verified it fails when the origin is dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`~> 6.0` already means `>= 6.0, < 7.0`, so the trailing `>= 5.0` was dead and misleadingly suggested reek 5 was supported. No change to which versions resolve. Verified the suite against reek 6.5.0, the current latest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Points the gem at the latest stable ruby-lsp (
0.26.10; 0.27 is still beta-only), drops the now-unusedsorbet-runtimedependency, and clears the reek warnings sorake code_analysispasses.ruby-lsp 0.26
ruby-lspdependency~> 0.17, >= 0.12.0→~> 0.26, andrequired_ruby_version>= 2.5.0→>= 3.0.0to match what ruby-lsp actually requires.Addon#version. Since 0.23 the base class treats it as abstract and uses it for theAddon.get(name, constraint)compatibility check, so without it any other addon querying this one blows up withAbstractMethodInvokedError.Everything else in the addon survived the jump —
register_formatter,run_diagnostic(uri, document),supports_watching_filesand the file-watcher registration all have unchanged signatures.The actual breakage was in the tests, which hand-rolled
with_serveragainst internals that no longer exist (core_ext/uri,RubyIndexer::IndexablePath). They now use theRubyLsp::TestHelpermodule ruby-lsp ships for addons, requireruby_lsp/internalinstead of a long explicit require list, and usepop_resultso notifications in the queue don't break assertions. The expected reek docs URL is interpolated fromReek::Version::STRINGrather than hardcoded atv6.3.0.Dropping sorbet-runtime
ruby-lsp 0.26 no longer depends on it, and the only reference left in this repo was a bare
require "sorbet-runtime"— noT.orT::Sigusage anywhere. Removing it stops forcing the gem on consumers.reek cleanup
rake code_analysiswas failing on 3 warnings (pre-existing, unrelated to the upgrade):FeatureEnvyinRunner#build_examiner— fixed by deleting the method. It existed to work aroundReek::Examinernot accepting a separate source and origin, doing it via twoinstance_variable_setcalls into reek internals.Reek::Source::SourceCode.from(source, origin:)takes an explicit origin andExaminerpasses aSourceCodethrough untouched, so the workaround collapses to one line with no internals poking.UtilityFunctionin#warning_to_diagnostic— took reek's own suggestion and extracted it toReek::Diagnostic.from_warningin a new file.UtilityFunctionin#run_formatting— not fixable. TheFormatterinterface mandates an instance method and reek is a linter, so there's nothing to format and no instance state to depend on. Suppressed in place with a comment explaining why, rather than a blanket.reek.ymlrule.Worth a look during review
build_examinerwas the fix from #5 and had no test, so rewriting it would have been blind. Addedtest_diagnostic_resolves_directives_from_the_file_path, which builds a tmpdir with a.reek.ymldirectory directive and asserts a file underapp/modelsis exempt while one underlibis not. Mutation-checked by droppingorigin:— the test fails, so it's a real regression test.Also note
lib/ruby_lsp/reek/diagnostic.rbhad to be git-tracked before it appeared in the built gem —spec.filescomes fromgit ls-files, so an untracked new file is silently omitted and the published gem would raiseLoadError. Confirmed viagem buildthat it's now packaged.Verification
rake code_analysis→0 total warnings.gem buildsucceeds with all four lib files packaged.🤖 Generated with Claude Code