Simplify Service attr helper methods #20391
Conversation
Library/Homebrew/service.rb
Outdated
| when String, Pathname | ||
| @working_dir = path.to_s | ||
| end | ||
| path ? @working_dir = path.to_s : @working_dir |
There was a problem hiding this comment.
I feel like an if/else would read a bit nicer here given the logic is mildly more complex than typical for a ternary.
dduugg
force-pushed
the
dug/service-refactor
branch
from
319b39c to
6c18f5c
Compare
| @@ -178,10 +173,9 @@ def keep_alive(value = nil) | |||
|
|
|||
| sig { params(value: T.nilable(T::Boolean)).returns(T::Boolean) } | |||
| def require_root(value = nil) | |||
| case value | |||
| when nil | |||
| if value.nil? | |||
There was a problem hiding this comment.
For boolean barams, we have to explicitly check .nil?, which inverts the logic. I could use this same approach for non-boolean param methods for consistency, if desirable.
There was a problem hiding this comment.
I think it's fine like this, thanks!
| @@ -178,10 +173,9 @@ def keep_alive(value = nil) | |||
|
|
|||
| sig { params(value: T.nilable(T::Boolean)).returns(T::Boolean) } | |||
| def require_root(value = nil) | |||
| case value | |||
| when nil | |||
| if value.nil? | |||
There was a problem hiding this comment.
I think it's fine like this, thanks!
Library/Homebrew/service.rb
Outdated
| sig { params(path: T.nilable(T.any(String, Pathname))).returns(T.nilable(String)) } | ||
| def working_dir(path = nil) |
There was a problem hiding this comment.
| sig { params(path: T.nilable(T.any(String, Pathname))).returns(T.nilable(String)) } | |
| def working_dir(path = nil) | |
| sig { params(path: T.any(String, Pathname)).returns(T.nilable(String)) } | |
| def working_dir(path = T.unsafe(nil)) |
@dduugg thoughts on this form?
There was a problem hiding this comment.
I'm fine with it, if you don't mind narrowing the existing contract. Lemme update the code, and we can see what breaks 😬
dduugg
force-pushed
the
dug/service-refactor
branch
from
3df90b6 to
02b6abc
Compare
| @@ -23,7 +23,7 @@ class Service | |||
| PROCESS_TYPE_ADAPTIVE = :adaptive | |||
|
|
|||
| KEEP_ALIVE_KEYS = [:always, :successful_exit, :crashed, :path].freeze | |||
| SOCKET_STRING_REGEX = %r{^([a-z]+)://(.+):([0-9]+)$}i | |||
| SOCKET_STRING_REGEX = %r{^(?<type>[a-z]+)://(?<host>.+):(?<port>[0-9]+)$}i | |||
There was a problem hiding this comment.
cc @carlocab 😎
(following through on #20324 (comment) )
dduugg
force-pushed
the
dug/service-refactor
branch
from
02b6abc to
899a6c5
Compare
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?Following through on the discussion at #20324 (comment)