writer: make internal identifiers private #504
Conversation
writer/multi_writer_test.go
Outdated
| @@ -163,7 +163,7 @@ func (m *mockPayloadSender) Reset() { | |||
| m.mu.Unlock() | |||
| } | |||
|
|
|||
| func (m *mockPayloadSender) Start() { | |||
| func (m *mockPayloadSender) start() { | |||
There was a problem hiding this comment.
Does it make sense to turn every method private? That way when using this class inside the writer package, it's not evident what constitutes the "public" interface of a sender vs what are internal helper methods. Perhaps it's sufficient to set the structs/interfaces themselves as private?
There was a problem hiding this comment.
As far as I know we should only export identifiers used in other packages. I don't think there is a concept of private and public at a structure level in Go. Even though I like your idea, and I did understand from the start that this was your intention, I'm not sure if it'll give the same impression to others too. I don't feel strongly about it and I could revert that part of the change if you wish, but I'd say it's not needed.
Additionally, I plan on coming back here and investigating a better way to test (e.g. by removing the syncBarrier and also the interfaces). I think you've initially added the interface only to facilitate testing and I think there might could be a simpler way.
I'll leave this decision up to you. I don't mind reverting if you feel it would make things clearer. But keep in mind that setEndpoint is the only method that is causing the confusion and I plan on removing it (it is only there to help with tests). After that work is done there will be no more confusion between actual methods and test helper methods.
There was a problem hiding this comment.
I'd prefer reverting because for example, the only safe methods for users of queueableSender are Start, Run, Stop, Send, NumQueuedPayloads and Monitor. If we put everything as lowercase, users might be tempted to use enqueue, sendOrQueue, flushQueue and others which were not coded with external calls in mind (lacking thread safety, etc...).
|
Ah, ok. I see what you mean now. I was looking at it only through the interface's methods, not through the entire structure. |
|
Done 👍 |
| // Write writes the payload to the endpoint. | ||
| Write(payload *Payload) error | ||
| write(payload *payload) error |
There was a problem hiding this comment.
As a matter of consistency we should strive for this uppercase public interface methods everywhere, what do you think?
There was a problem hiding this comment.
I wouldn't want to enforce that. I get the logic in the sender because of setEndpoint which is specifically there for tests, but I wouldn't want to make this a rule. If we capitalise these, it means that technically they can be accessed from outside the package too if this interface would be returned anywhere.
|
Thanks for helping out. Indeed |
writeridentifiers privateBasePayloadSenderintoQueuablePayloadSenderQueuablePayloadSendertoqueuableSender.