Various gem RBI LSP runner refactors #2177
Conversation
|
|
||
| sig { returns(Pathname) } | ||
| def lockfile | ||
| @lockfile ||= T.let(Pathname(project_path).join("Gemfile.lock"), T.nilable(Pathname)) |
There was a problem hiding this comment.
I couldn't get Bundler.with_original_env { Bundler.default_lockfile } to work in tests. If I do a Dir.chdir(@project.absolute_path) in tests, then it works fine, but the whole idea was to be cwd-independent, so not sure if we can use it or not.
At least I've extracted the method that sets up the lockfile path, so that the change can be isolated.
There was a problem hiding this comment.
I tested this and it's not cleaning up orphaned RBIs. Did it succeed for you in core?
There was a problem hiding this comment.
Wait, I don't understand, this doesn't change anything from the old code.
There was a problem hiding this comment.
Yeah it could be a problem with the old version. I'm just trying to make sure we aren't introducing a regression while refactoring. I'll do more tests.
There was a problem hiding this comment.
Problem was that we only cleanup orphan RBIs if the lockfile hasn't changed. Which isn't correct. Not a regression.
paracycle
added
refactor
paracycle
force-pushed
the
uk-improve-gem-lsp
branch
from
aa1a48f to
80e27c8
Compare
| end | ||
|
|
||
| private | ||
|
|
||
| attr_reader :project_path | ||
|
|
||
| sig { returns(T::Boolean) } | ||
| sig { returns(T.nilable(T::Boolean)) } |
There was a problem hiding this comment.
I still think this shouldn't return a nil. The caller only needs to know if it's true or false. Feels like this is leaking an implementation detail of Process::Status.
There was a problem hiding this comment.
It does know if it is true or false, just not true or false.
Ruby is a simple language: Only nil and false are false values, everything else is true. That's why there is no concept of a "Boolean" in Ruby.
So, a predicate method can return any value that evaluates to true.
For example, what do you think of this code?
def valid?
user.name && user.age
endThis is very idiomatic Ruby code, since the value value doesn't matter, as long as both booleans are satisfied, the method returns a true value. But it doesn't necessarily return true. For example, for a valid user with a name and an age, this will return the age value as the result of valid?, which is perfectly fine, since it is a true value.
Finally, I am just returning the result of status.success? which itself returns true or false or nil. If predicate methods aren't supposed to return nil, then why did Ruby choose to do that for Process::Status#success? and how is this method any different?
|
|
||
| sig { returns(Pathname) } | ||
| def lockfile | ||
| @lockfile ||= T.let(Pathname(project_path).join("Gemfile.lock"), T.nilable(Pathname)) |
There was a problem hiding this comment.
Problem was that we only cleanup orphan RBIs if the lockfile hasn't changed. Which isn't correct. Not a regression.
Improves the codebase