-
-
Notifications
You must be signed in to change notification settings - Fork 270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[bridge] Change bridge name format in the config #170
Conversation
bridge/core/bridge.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
bridge.conf = conf | ||
|
||
target := bridge.conf[keyTarget] |
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.
That's pretty much the implementation of NewBridge()
, so why the need to copy it instead of using it ? If you want to change the API to have LoadBridge instead, you should remove NewBridge() or at least deprecate it.
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.
The problem i encountered while writing this was that LoadBridge
need to read the configuration before instantiating a bridge object, and since loadConfig()
is a method of Bridge
i was forced to do this.
NewBridge
is used by git bug bridge configure
who already knows the target name and want to instantiate a bridge directly.
What do you think of making loadConfig
a more general function: something like loadConfig(repo *cache.Repo, name string)
and then call NewBridge
from LoadBridge
to avoid code repetition ?
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.
sounds good to me
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.
Done !
update loadConfig signature
dealing with #157