chore: add additional comments to code/structs, and sanity tests to verify functions are called within the expected context #151
Conversation
jgwest
force-pushed
the
add-comments-to-repo-aug-2024
branch
from
4812652
to
b9f765d
Compare
jgwest
changed the title
jgwest
changed the title
jgwest
force-pushed
the
add-comments-to-repo-aug-2024
branch
from
4bfde1a
to
6c1efa1
Compare
Signed-off-by: Jonathan West <[email protected]>
jgwest
force-pushed
the
add-comments-to-repo-aug-2024
branch
from
6c1efa1
to
492afa7
Compare
| @@ -238,29 +254,30 @@ func (s *Server) loadTLSConfig() (*tls.Config, error) { | |||
| return tlsConfig, nil | |||
| } | |||
|
|
|||
| // Listener returns the listener of Server s | |||
| func (s *Server) Listener() *Listener { | |||
| // ListenerForE2EOnly returns the listener of Server s | |||
There was a problem hiding this comment.
For all the *E2EOnly field changes here:
When I initially saw these struct fields, I noticed these fields are not exported from the struct, which implies they are for internal package use only.
BUT, then I noticed we are making the contents of these fields available by exporting these functions. For example, Queues() could be called to retrieve the internal queues reference of the principal.
Finally, I noticed that they were only being used for the E2E tests, so I have renamed these functions to allow them to still be used by E2E tests, but to make it obvious they are not for general use.
(If you were expecting these functions WOULD be available for general use, or just generally don't like the name scheme, I'm happy to revert this part of the PR)
There was a problem hiding this comment.
Good points there, great observations. I think at some point in time we made use of at least the Listener function, but since then move forward do something else.
I guess I'm fine with renaming those functions for now.
| @@ -75,7 +76,7 @@ func (a *Agent) sender(stream eventstreamapi.EventStream_SubscribeClient) error | |||
| } | |||
| logCtx.Tracef("Grabbed an item") | |||
| if item == nil { | |||
| // FIXME: Is this really the right thing to do? | |||
| // TODO: Is this really the right thing to do? | |||
There was a problem hiding this comment.
Changed FIXME to TODO. This seemed to be the only instance of FIXME in the code. Happy to revert this part of the PR if FIXME was intentional.
| @@ -133,26 +134,6 @@ func (a *Agent) receiver(stream eventstreamapi.EventStream_SubscribeClient) erro | |||
| if err != nil { | |||
| logCtx.WithError(err).Errorf("Unable to process incoming event") | |||
| } | |||
| // switch ev.Type() { | |||
There was a problem hiding this comment.
This code was moved into its own function which is called above, and is no longer needed.
This PR: