clean up comments

Author: robbyt

examples/composite/worker.go

@@ -82,7 +82,7 @@ func NewWorker(config WorkerConfig, logger *slog.Logger) (*Worker, error) {
 	}, nil
 }
 
-// String returns the worker's name and interval safely.
+// String returns the worker's name and interval.
 func (w *Worker) String() string {
 	return fmt.Sprintf("Worker{config: %s}", w.getConfig())
 }
@@ -222,14 +222,14 @@ func (w *Worker) ReloadWithConfig(config any) {
 	}
 }
 
-// getConfig returns the worker's current configuration safely.
+// getConfig returns the worker's current configuration.
 func (w *Worker) getConfig() WorkerConfig {
 	w.mu.Lock()
 	defer w.mu.Unlock()
 	return w.config
 }
 
-// setConfig updates the internal configuration state safely.
+// setConfig updates the internal configuration state.
 // It returns the *previous* config object.
 func (w *Worker) setConfig(newConfig WorkerConfig) (oldCfg WorkerConfig) {
 	w.mu.Lock()

examples/http/main.go

@@ -14,7 +14,7 @@ import (
 )
 
 const (
-	// Port the http server should bind to
+	// Port the http server binds to
 	ListenOn = ":8080"
 
 	// How long the supervisor waits for the HTTP server to drain before forcefully shutting down
@@ -71,7 +71,7 @@ func buildRoutes(logHandler slog.Handler) ([]httpserver.Route, error) {
 		return nil, fmt.Errorf("failed to create status route: %w", err)
 	}
 
-	// API routes need their middlewares passed explicitly with the updated function
+	// API routes have their middlewares passed to the updated function
 	apiRoute, err := httpserver.NewWildcardRoute("/api", wildcardHandler, commonMw...)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create wildcard route: %w", err)

internal/networking/port.go

@@ -15,8 +15,8 @@ var (
 	ErrPortOutOfRange = errors.New("port number must be between 1 and 65535")
 )
 
-// ValidatePort checks if the provided port string is valid and returns a clean, normalized version.
-// It accepts formats like ":8080", "localhost:8080", "127.0.0.1:8080", or just "8080".
+// ValidatePort checks if the provided port string is valid and returns a normalized version.
+// It accepts formats like ":8080", "localhost:8080", "127.0.0.1:8080", or "8080".
 // Returns the normalized port format (":PORT") and an error if the port is invalid.
 func ValidatePort(portStr string) (string, error) {
 	if portStr == "" {
@@ -29,12 +29,12 @@ func ValidatePort(portStr string) (string, error) {
 		return "", fmt.Errorf("%w: negative port numbers are not allowed", ErrInvalidFormat)
 	}
 
-	// If it's just a number, add a colon prefix
+	// If it's a number without a colon, add a colon prefix
 	if !strings.Contains(portStr, ":") {
 		portStr = ":" + portStr
 	}
 
-	// Handle host:port format by extracting just the port
+	// Handle host:port format by extracting the port
 	host, port, err := net.SplitHostPort(portStr)
 	if err != nil {
 		return "", fmt.Errorf("%w: %w", ErrInvalidFormat, err)

runnables/composite/config.go

@@ -8,7 +8,7 @@ import (
 	"github.com/robbyt/go-supervisor/supervisor"
 )
 
-// runnable is a local alias to ensure the sub-runnables implement the
+// runnable is a local alias constraining sub-runnables to implement the
 // supervisor.Runnable interface.
 type runnable interface {
 	supervisor.Runnable

runnables/composite/reload.go

@@ -98,13 +98,13 @@ func (r *Runner[T]) reloadMembershipChanged(newConfig *Config[T]) error {
 // reloadConfig handles the case where the membership of runnables has not changed.
 func (r *Runner[T]) reloadConfig(logger *slog.Logger, newConfig *Config[T]) {
 	logger = logger.WithGroup("reloadConfig")
-	// No membership change, just update config and reload existing runnables
+	// No membership change, update config and reload existing runnables
 	r.configMu.Lock()
 	r.setConfig(newConfig)
 	r.configMu.Unlock()
 
 	// Reload configs of existing runnables
-	// No need to lock the runnables mutex as we're not changing membership
+	// Runnables mutex not locked as membership is not changing
 	for _, entry := range newConfig.Entries {
 		if reloadableWithConfig, ok := any(entry.Runnable).(ReloadableWithConfig); ok {
 			// If the runnable implements our ReloadableWithConfig interface, use that to pass the new config

runnables/composite/runner.go

@@ -36,7 +36,7 @@ type Runner[T runnable] struct {
 //   - configCallback: Required. A function that returns the initial configuration and is called during any reload operations.
 //   - opts: Optional. A variadic list of Option functions to customize the Runner behavior.
 //
-// The configCallback must not be nil and will be invoked by Run() to load the initial configuration.
+// The configCallback cannot be nil and will be invoked by Run() to load the initial configuration.
 func NewRunner[T runnable](
 	configCallback ConfigCallback[T],
 	opts ...Option[T],
@@ -60,7 +60,7 @@ func NewRunner[T runnable](
 		opt(r)
 	}
 
-	// Validate requirements
+	// Validate configuration
 	if r.configCallback == nil {
 		return nil, fmt.Errorf(
 			"%w: config callback is required",
@@ -175,15 +175,15 @@ func (r *Runner[T]) boot(ctx context.Context) error {
 		return fmt.Errorf("%w: configuration is unavailable", ErrConfigMissing)
 	}
 
-	// If there are no entries, just log and return successfully instead of error
+	// If there are no entries, log and return without error
 	if len(cfg.Entries) == 0 {
 		logger.Debug("No runnables found in configuration")
 		return nil
 	}
 
 	logger.Debug("Starting child runnables...", "count", len(cfg.Entries))
 
-	// Use a temporary WaitGroup to ensure all goroutines are launched.
+	// Use a temporary WaitGroup to track that all goroutines have started.
 	var startWg sync.WaitGroup
 	startWg.Add(len(cfg.Entries))
 	for i, e := range cfg.Entries {

runnables/composite/state.go

@@ -32,8 +32,8 @@ func (r *Runner[T]) setStateError() {
 
 // GetChildStates returns a map of child runnable names to their states.
 func (r *Runner[T]) GetChildStates() map[string]string {
-	// No need for runnables lock, just reading config and querying state
-	// which doesn't modify any internal state
+	// Runnables lock not required, reading config and querying state
+	// does not modify any internal state
 
 	states := make(map[string]string)
 	cfg := r.getConfig()

runnables/httpcluster/entries.go

@@ -35,7 +35,7 @@ type serverEntry struct {
 var _ entriesManager = (*entries)(nil)
 
 // entries is an immutable collection of server entries with pending actions.
-// Once created, it should not be modified - only replaced with a new instance.
+// Once created, it is not modified - only replaced with a new instance.
 type entries struct {
 	servers map[string]*serverEntry
 }
@@ -59,7 +59,7 @@ func newEntries(desiredConfigs map[string]*httpserver.Config) *entries {
 	return &entries{servers: servers}
 }
 
-// removeEntry creates a new entries collection with the specified entry completely removed.
+// removeEntry creates a new entries collection with the specified entry removed.
 func (e *entries) removeEntry(id string) entriesManager {
 	_, exists := e.servers[id]
 	if !exists {
@@ -111,7 +111,7 @@ func (e *entries) countByAction(a action) int {
 }
 
 // commit creates a new entries collection with all actions marked as complete.
-// This should be called after all pending actions have been executed.
+// Called after all pending actions have been executed.
 // It removes entries marked for stop and clears all action flags.
 func (e *entries) commit() entriesManager {
 	servers := make(map[string]*serverEntry)
@@ -212,7 +212,7 @@ func processExistingServer(
 			return
 		}
 
-		// Case 1: Server should be removed
+		// Case 1: Server is removed
 		if desiredConfig == nil {
 			if oldEntry.runner != nil {
 				newEntry := *oldEntry
@@ -233,7 +233,7 @@ func processExistingServer(
 
 		// Case 3: Config changed - need to restart
 		if oldEntry.runner != nil {
-			// Running server needs restart
+			// Running server marked for restart (stop then start)
 			stopEntry := *oldEntry
 			stopEntry.action = actionStop
 

runnables/httpcluster/interfaces.go

@@ -21,7 +21,7 @@ type entriesManager interface {
 	getPendingActions() (toStart, toStop []string)
 
 	// commit creates a new entries collection with all actions marked as complete.
-	// This should be called after all pending actions have been executed.
+	// Called after all pending actions have been executed.
 	// It removes entries marked for stop and clears all action flags.
 	commit() entriesManager
 
@@ -40,8 +40,8 @@ type entriesManager interface {
 	// Returns nil if the server doesn't exist.
 	clearRuntime(id string) entriesManager
 
-	// removeEntry creates a new entries collection with the specified entry completely removed.
-	// This is used when a server fails to start and should be completely removed.
+	// removeEntry creates a new entries collection with the specified entry removed.
+	// This is used when a server fails to start and is removed from the collection.
 	removeEntry(id string) entriesManager
 
 	// buildPendingEntries creates a new entries collection based on the desired state and the previous state.

runnables/httpcluster/runner.go

@@ -305,7 +305,7 @@ func (r *Runner) executeActions(ctx context.Context, pending entriesManager) ent
 	if len(toStop) > 0 {
 		current = r.stopServers(ctx, current, toStop)
 
-		// Delay after stopping servers to allow OS to fully release sockets
+		// Delay after stopping servers to allow OS to release sockets
 		// before binding to the same ports again
 		if len(toStart) > 0 && r.restartDelay > 0 {
 			logger.Debug("Waiting for ports to be released", "delay", r.restartDelay)
@@ -397,7 +397,7 @@ func (r *Runner) startServers(
 		runner, serverCtx, serverCancel, err := r.createAndStartServer(ctx, entry)
 		if err != nil {
 			logger.Error("Failed to create server", "error", err)
-			// Remove entry completely since server failed to create
+			// Remove entry since server failed to create
 			current = current.removeEntry(id)
 			continue
 		}
@@ -414,9 +414,8 @@ func (r *Runner) startServers(
 				"state", runner.GetState())
 			// Cancel the server context to clean up
 			serverCancel()
-			// Stop the runner explicitly to ensure cleanup
 			runner.Stop()
-			// Remove entry completely since server failed to start
+			// Remove entry since server failed to start
 			current = current.removeEntry(id)
 			continue
 		}

runnables/httpserver/config.go

@@ -107,7 +107,7 @@ func WithRequestContext(ctx context.Context) ConfigOption {
 }
 
 // WithConfigCopy creates a ConfigOption that copies most settings from the source config
-// except for ListenAddr and Routes which must be provided directly to NewConfig.
+// except for ListenAddr and Routes which are provided directly to NewConfig.
 func WithConfigCopy(src *Config) ConfigOption {
 	return func(dst *Config) {
 		if src == nil {
@@ -126,7 +126,7 @@ func WithConfigCopy(src *Config) ConfigOption {
 	}
 }
 
-// NewConfig creates a new Config with the required address and routes
+// NewConfig creates a new Config with the address and routes
 // plus any optional configuration via functional options
 func NewConfig(addr string, routes Routes, opts ...ConfigOption) (*Config, error) {
 	if len(routes) == 0 {

runnables/httpserver/middleware/metrics.go

@@ -7,7 +7,7 @@ import (
 )
 
 // MetricCollector creates a middleware that collects metrics about HTTP requests.
-// This is a placeholder implementation that should be extended to integrate with
+// This is a placeholder implementation that can be extended to integrate with
 // your metrics collection system.
 func MetricCollector() Middleware {
 	return func(next http.HandlerFunc) http.HandlerFunc {
@@ -31,7 +31,7 @@ func MetricCollector() Middleware {
 			// httpRequestsTotal.WithLabelValues(r.Method, r.URL.Path, strconv.Itoa(rw.statusCode)).Inc()
 			// httpRequestDuration.WithLabelValues(r.Method, r.URL.Path).Observe(duration.Seconds())
 
-			// For now, just log the metrics
+			// Log the metrics
 			slog.Debug("HTTP request metrics",
 				"method", r.Method,
 				"path", r.URL.Path,

runnables/httpserver/options.go

@@ -31,8 +31,8 @@ func WithContext(ctx context.Context) Option {
 	}
 }
 
-// WithConfigCallback sets the function that will be called to load or reload configuration. Using
-// this option or WithConfig is required to initialize the Runner instance, because it provides the
+// WithConfigCallback sets the function that will be called to load or reload configuration.
+// Either this option or WithConfig initializes the Runner instance by providing the
 // configuration for the HTTP server managed by the Runner.
 func WithConfigCallback(callback ConfigCallback) Option {
 	return func(r *Runner) {

runnables/httpserver/routes.go

@@ -32,7 +32,6 @@ func applyMiddlewares(
 }
 
 // NewRoute creates a new Route with the given name, path, and handler.
-// The name must be unique, the path must be non-empty, and the handler must not be nil.
 func NewRoute(name string, path string, handler http.HandlerFunc) (*Route, error) {
 	if name == "" {
 		return nil, errors.New("name cannot be empty")

runnables/httpserver/runner.go

@@ -16,7 +16,7 @@ import (
 	"github.com/robbyt/go-supervisor/supervisor"
 )
 
-// Interface guards to ensure all of these are implemented
+// Interface guards verify implementation at compile time
 var (
 	_ supervisor.Runnable   = (*Runner)(nil)
 	_ supervisor.Reloadable = (*Runner)(nil)

supervisor/interfaces.go

@@ -23,13 +23,13 @@ import (
 
 // Runnable represents a service that can be run and stopped.
 type Runnable interface {
-	fmt.Stringer // Runnables needs a String() method to be identifiable in logs
+	fmt.Stringer // Runnables implement a String() method to be identifiable in logs
 
 	// Run starts the service with the given context and returns an error if it fails.
-	// Run must be a blocking call that runs the work unit until it is stopped.
+	// Run is a blocking call that runs the work unit until it is stopped.
 	Run(ctx context.Context) error
 	// Stop signals the service to stop.
-	// Stop must be a blocking call that stops the work unit.
+	// Stop is a blocking call that stops the work unit.
 	Stop()
 }
 

supervisor/state.go

@@ -216,7 +216,7 @@ func (p *PIDZero) startStateMonitor() {
 
 						prev, loaded := p.stateMap.Swap(r, state)
 						if !loaded {
-							// The state map entry for this runnable should have been created elsewhere
+							// The state map entry for this runnable was expected to be created in startRunnable
 							p.logger.Warn(
 								"Unexpected State map entry created",
 								"runnable", r,

supervisor/supervisor.go

@@ -237,7 +237,6 @@ func (p *PIDZero) Run() error {
 }
 
 // blockUntilRunnableReady blocks until the runnable is in a running state.
-// This requires implementing the Stateable interface so the runnable can be monitored.
 func (p *PIDZero) blockUntilRunnableReady(r Stateable) error {
 	startupCtx, cancel := context.WithTimeout(p.ctx, p.startupTimeout)
 	defer cancel()
@@ -343,7 +342,7 @@ func (p *PIDZero) Shutdown() {
 					"elapsed", time.Since(shutdownStart))
 			}
 		} else {
-			// No timeout configured, just wait
+			// No timeout configured, wait indefinitely
 			p.wg.Wait()
 		}