Fixes #8016 - api connection moved to context #227
Conversation
tstrachota
force-pushed
the
sessions_8016
branch
2 times, most recently
from
e91c5ab to
45090d5
Compare
- connection moved to context - connection is now instance, not class - dependency injection for connection and connectors
tstrachota
force-pushed
the
sessions_8016
branch
from
45090d5 to
dd523b1
Compare
| :defaults => HammerCLI.defaults, | ||
| :is_tty? => HammerCLI.tty? | ||
| :is_tty? => HammerCLI.tty?, | ||
| :api_connection => HammerCLI::Connection.new(Logging.logger['Connection']) |
There was a problem hiding this comment.
Would it make sense to move this to command initializer and add just the attributes missing? This way you could provide only part of the context to the command and the rest would be replaced with the defaults. The context would also be accessible with @context?
There was a problem hiding this comment.
I'm not sure I understand. The context is passed to commands' constructors and is accessible as self.context or @context.
I don't think setting a default value for the api connection makes sense in this case, because we need one connection for the whole hammer.
There was a problem hiding this comment.
I see, sorry for the confusion.
|
The context is not blocker for getting this merged. It just occurred to me that while Clamp allows to provide the context as command parameter. We would not be able to add the api_connection when it is missing there. I'd like to know if you think it should be fixed in this PR. Otherwise I'm going to merge it. |
|
👍 Thanks @tstrachota! |
TODO: