Extract commonly used objects into a separate struct #345
Conversation
oxzi
force-pushed
the
icingadb-source
branch
4 times, most recently
from
e2a1836 to
f3870a3
Compare
yhabteab
force-pushed
the
minor-enhancements
branch
2 times, most recently
from
9a81611 to
5d0d19c
Compare
julianbrost
left a comment
julianbrost
left a comment
There was a problem hiding this comment.
General question: You chose to embed *Resources into other structs instead of having it as a regular member. I believe you did that so you can just write x.DB instead of something like x.res.DB. However, doing it that way has two other drawbacks in my opinion:
- This makes code navigation a bit harder. If it's a regular member, I can find all uses easily, whereas now, answering a question like "which members of
Resourcesare actually needed/used byRuntimeConfig?" (there's also an individual comment on that) isn't that easy. - It implicitly makes them part of the public interface, i.e.
x.DBis exported, whereasx.res.DBisn't.
| func (c *Channel) Start(ctx context.Context, conf *daemon.ConfigFile, logger *zap.SugaredLogger) { | ||
| c.Logger = logger.With(zap.Object("channel", c)) | ||
| c.daemonConfig = conf |
There was a problem hiding this comment.
Why is this passed individually instead of as part of *Resources (as I'd kind of expect from the overall vibe of this PR)?
There was a problem hiding this comment.
Because Resources is within the config package and imports this channel package.
internal/config/runtime.go
Outdated
| // Accessing it requires a lock that is obtained with RLock() and released with RUnlock(). | ||
| ConfigSet | ||
|
|
||
| *Resources // Contains references to commonly used objects and to itself. |
There was a problem hiding this comment.
To itself? That sounds a bit weird. I mean technically it does given that Resources contains a *RuntimeConfig, but should this ever be accessed from here?
There was a problem hiding this comment.
No, why would you ever want to access it that way. But fine, I'll adjust the comment.
That's how Go works and isn't a drawback for me. If you for whatever reason end up asking that question to yourself, then you can list all usages of the fields of Resources with your IDE or grep, but that's not a reason to avoid embedding it IMHO.
That's true, but I don't see a problem with that. This new struct contains nothing that's specific to some other type, it's just a bag of global resources. So exposing them via embedding is fine for me. |
It's getting ridiculous that those fields have to be passed around everywhere into functions and methods. This becomes even more ridiculous with upcoming PR of mine, so I just bite the bullet and make a struct for it. This struct is typically embedded into other structs that need access to these common resources, and each function or method has just to take the `RuntimeConfig` as a parameter, and this will be populated from there.
yhabteab
force-pushed
the
minor-enhancements
branch
from
5d0d19c to
5585b9e
Compare
|
@cla-bot check |
|
I've looked at this for a while now and I'm not yet convinced that this is really the way to go. Sure, it allows you to pass a single argument instead of two or three in some places, but it also looks like it introduces quite some trouble with cyclic imports. At least it looks to my like the explanation to why the new Regarding the commit message of 9fb2aa3:
As I haven't looked at that (now closed) PR in detail, I don't know what exactly changed there and to what extent it would have proved the necessity of this change, but on it's own, I'm not sure. On the other hand, if passing dependencies is getting ridiculous, that might also be an opportunity to think about if the dependency itself is ridiculous and could be avoided. In the changes of this PR, I already noticed a small example that shows this: Why does icinga-notifications/internal/channel/channel.go Lines 176 to 177 in 5585b9e |
These commits are just left overs from my previous PRs but can independently be merged, since they don't depend on them and enhance some of the existing code flows.