Map generator -verbose and -performance flags #2721
Conversation
WalkthroughThreads context.Context and a slog-based, flag-driven logger through the map-generator, adds CLI flags and per-map contextual loggers, updates GenerateMap and many helpers to accept ctx, and documents new flags in README. No map-generation algorithms were changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI
participant LoggerInit as Logger Init
participant Main
participant Generate as GenerateMap(ctx)
participant Helpers as Processing Helpers
participant Stdout
User->>CLI: run tool with flags (--maps, --log-level, --verbose, --debug-*)
CLI->>LoggerInit: build LogFlags & NewGeneratorLogger(out, opts, flags)
LoggerInit-->>CLI: install GeneratorLogger as default slog handler
CLI->>Main: start per-map goroutines for selected maps
Main->>LoggerInit: create per-map slog.Logger & ContextWithLogger(mapName)
Main->>Generate: call GenerateMap(ctxWithLogger, GeneratorArgs)
Generate->>Helpers: call processing helpers with ctx (processWater, packTerrain, createThumbnail, ...)
Helpers-->>Generate: return results / errors (emit logs via ctx-logger)
Generate->>Stdout: logs routed through GeneratorLogger (filtered/prefixed by flags)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
map-generator/map_generator.go (1)
94-110: Rename parameter to avoid shadowing the type name.The parameter name
LogArgson line 97 shadows the type nameLogArgs, which can be confusing for readers. While Go allows this, it reduces code clarity.🔎 Suggested fix
- return func(LogArgs LogArgs, format string, v ...interface{}) { - if LogArgs.isVerbose && !args.Verbose { + return func(logArgs LogArgs, format string, v ...interface{}) { + if logArgs.isVerbose && !args.Verbose { return } - if LogArgs.isPerformance && !args.Performance { + if logArgs.isPerformance && !args.Performance { return }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
map-generator/README.mdmap-generator/main.gomap-generator/map_generator.go
🔇 Additional comments (10)
map-generator/main.go (3)
16-17: LGTM! Flag declarations follow Go conventions.The package-level flag variables follow the same pattern as the existing
mapsFlagand use clear, descriptive names.
136-137: Correct flag usage with proper test map exclusion.The logic correctly passes the verbose flag to all maps while excluding test maps from performance warnings using
performanceFlag && !isTest. This aligns with the PR objectives.
253-255: LGTM! Idiomatic flag registration.Registering both
--verboseand-vto the same variable is the standard Go pattern for providing shorthand flags. The help descriptions are clear and concise.map-generator/README.md (1)
36-40: LGTM! Clear and complete documentation.The new "Command Line Flags" section clearly documents all available flags, including both the longhand and shorthand forms for the verbose option. The descriptions are concise and helpful.
map-generator/map_generator.go (6)
21-28: LGTM! Constants align with documented recommendations.The performance threshold constants match the README's guidance on map dimensions (2-3 million pixels) and provide clear, self-documenting values for the performance checks.
85-92: Clean logging abstraction.The
LogArgsstruct andLoggerfunction type provide a simple, pluggable logging system that supports conditional output based on verbose and performance flags. The design is straightforward and effective.
81-83: LGTM! Clean extension of GeneratorArgs.The new
VerboseandPerformanceboolean fields integrate cleanly into the existing struct without breaking changes.
150-155: Correct performance check implementation.The area calculation and range check correctly implement the performance warning requirement. The check is non-fatal and only runs when the performance flag is enabled, as specified in the PR objectives.
210-219: Comprehensive land tile count logging.The verbose logs provide the requested land tile counts at all scales (1x, 4x, 16x), and the performance warnings catch both edge cases (zero land tiles and excessive land tiles). This fulfills the PR objectives.
303-303: Consistent logger propagation throughout the codebase.The logger parameter has been systematically added to all relevant functions, with appropriate usage of verbose and performance flags at each call site. This creates a clean, consistent logging approach across the entire map generation workflow.
Also applies to: 341-341, 423-423, 527-527, 584-584, 623-623, 727-727
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
map-generator/map_generator.go (3)
94-110: Small nit: incomplete comment on line 96.The comment
// and handle checking against the performance and verbose flagsstarts with "and" which suggests it was meant to continue from somewhere. Consider making it a complete sentence.🔎 Suggested fix
// createLogger creates a logger instance that handles formatting and conditional output +// based on the performance and verbose flags. func createLogger(args GeneratorArgs) Logger { - // and handle checking against the performance and verbose flags return func(logArgs LogArgs, format string, v ...interface{}) {
149-155: Redundant check: the logger already handles this.The
createLoggerfunction already skips logs whenlogArgs.isPerformanceis true butargs.Performanceis false. The outerif args.Performancewrapper is not needed.🔎 Suggested fix
- // only perform this check if the performance flag is enabled - if args.Performance { - area := width * height - if area < minRecommendedPixelSize || area > maxRecommendedPixelSize { - logger(LogArgs{isPerformance: true}, "⚠️ Map area %d pixels is outside recommended range (%d - %d)", area, minRecommendedPixelSize, maxRecommendedPixelSize) - } + area := width * height + if area < minRecommendedPixelSize || area > maxRecommendedPixelSize { + logger(LogArgs{isPerformance: true}, "⚠️ Map area %d pixels is outside recommended range (%d - %d)", area, minRecommendedPixelSize, maxRecommendedPixelSize) }
727-737: Consider making binary data logging verbose-only.The
logBinaryAsBitsfunction logs raw binary data which is mainly useful for debugging. When running many maps, this output could be noisy. Consider gating it behind the verbose flag.🔎 Suggested fix
- logger(LogArgs{}, "Binary data (bits): %s", bits) + logger(LogArgs{isVerbose: true}, "Binary data (bits): %s", bits)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/map_generator.go
🔇 Additional comments (9)
map-generator/map_generator.go (9)
21-28: Looks good!Constants are clear, well-named, and easy to understand. Good use of separate const blocks for grouping related values.
81-92: Clean type design.The
Loggerfunction type andLogArgsstruct are simple and easy to follow. Using a function type instead of an interface keeps things lightweight.
206-219: Good use of logging levels.The land tile count logs are correctly marked as verbose, and the performance warnings use the performance flag. This matches the PR objectives well.
303-304: Clean update.Function signature updated to accept the logger. The log message is always shown which is appropriate for this processing step.
341-342: Looks good.
423-492: Good implementation of verbose logging for water body processing.The detailed removal coordinates (line 476) are correctly gated behind the verbose flag, while summary information remains always visible. This matches the PR objectives well.
527-573: Consistent with water body handling.Same pattern as
processWater: detailed per-island removal is verbose, summary is always shown. Good consistency.
584-617: Clean update to packTerrain.
623-624: Looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
map-generator/map_generator.go (1)
728-738: Binary logging should be verbose-only.The
logBinaryAsBitsfunction logs with emptyLogArgs{}, meaning binary data is always logged regardless of verbose mode. This appears in the output for all three scales (1x, 4x, 16x) during every map generation.Binary representation of packed terrain is debugging information that should only appear when
-verboseis enabled, per the PR objective to preserve default behavior.🔎 Proposed fix
- logger(LogArgs{}, "Binary data (bits): %s", bits) + logger(LogArgs{isVerbose: true}, "Binary data (bits): %s", bits)
🧹 Nitpick comments (1)
map-generator/map_generator.go (1)
85-89: Consider exporting LogArgs fields for clarity.The
LogArgsstruct is exported but its fieldsisVerboseandisPerformanceare unexported. This prevents external packages from setting these fields directly and is an unconventional Go pattern. Since all usage appears internal to this package, consider either:
- Exporting the fields (rename to
IsVerbose,IsPerformance)- Or unexport the struct entirely (rename to
logArgs)The current design works but may confuse future maintainers expecting standard Go conventions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
map-generator/map_generator.go (2)
85-114: LGTM: Well-designed logger abstraction.The closure-based logger cleanly captures the generator args and provides conditional output based on both message type and global flags. The name prefixing in verbose mode matches the PR requirements exactly.
Optional style notes:
- Lines 98, 102: "dont" → "don't" in comments
- Line 108: The nested
fmt.Sprintfcalls are clear but could be micro-optimized by building a single format stringThese are minor style points that don't affect functionality.
477-477: Optional: Consider defensive check for empty coordinates.Accessing
coords[0]is safe here becausegetArea()always includes the starting tile. However, the invariant is subtle—a defensive check or comment would help future maintainers:Optional defensive pattern
if waterBodies[w].size < minLakeSize { + if len(waterBodies[w].coords) == 0 { + continue // Safety: skip empty body + } logger(loggerArgs{isVerbose: true}, "Removing small lake at %d,%d (size %d)", waterBodies[w].coords[0].X, waterBodies[w].coords[0].Y, waterBodies[w].size)Also applies to: 564-564
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/map_generator.go
🔇 Additional comments (6)
map-generator/map_generator.go (6)
21-28: LGTM: Clear performance thresholds.The constants provide sensible boundaries for map area and tile counts, with clear naming that makes their purpose immediately obvious.
77-83: LGTM: Clean flag additions to GeneratorArgs.The new boolean fields integrate cleanly with the existing struct and directly map to the CLI flags described in the PR objectives.
153-156: LGTM: Proper area bounds checking.The performance check correctly validates map area against both minimum and maximum thresholds, using the
isPerformanceflag so warnings only appear when requested.
215-220: LGTM: Comprehensive land tile validation.The checks catch both edge cases (zero land tiles indicating potential data issues) and performance concerns (excessive tiles), with clear warnings that respect the
--performanceflag.
140-140: LGTM: Clean logger propagation throughout the call chain.The logger is properly threaded through all processing functions with consistent signatures. This enables centralized control over logging behavior while keeping the implementation clean and testable.
Also applies to: 188-196, 304-305, 342-343, 424-425, 490-491, 528-528, 585-585, 617-617, 624-625, 728-728
12-12: WebP library dependency is secure and current.Version 1.4.0 is the latest release and includes the security fix for CVE-2023-4863 (out-of-bounds/heap buffer issue in libwebp). No further action needed.
|
This is good to review. This is isolated from and does not include the changes from #2709 - but if the recommendations in the README change over there, the logical checks here should be updated (note for future me, really)... once they are both merged they can always get updated in sync. The only implementation difference from the issue description is using a closure proxy instead of a bunch of formatter calls. I think this will scale better for future flags that might want to enable additional logging, or help to sort some of the current logging if that is desired ( @coderabbitai really wants the binary stuff behind the verbose flag, but i kept it always logging so the default output wouldnt change) |
|
@Tidwell: Thanks for the heads-up! I'll review the changes now. Your implementation approach using a closure proxy makes sense for scalability—it gives you a clean, centralized point to control logging behavior and makes it straightforward to add new flag-driven logging options in the future without scattering conditional logic throughout the codebase. I appreciate you preserving the default output behavior by keeping the binary logging always on. That's the right call for this PR since it maintains backward compatibility and doesn't surprise existing users of the tool. Good note about coordinating with PR #2709 once both are merged—keeping those README recommendations and the logical checks in sync will be important. Let me review the implementation now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Tidwell
force-pushed
the
map-generator-verbose
branch
from
42fee8f to
60a193d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
map-generator/logger.go (2)
60-61: Consider writing the invalid log level warning to stderr.When a user provides an invalid value, writing to
os.Stderrmakes it clearer this is a warning rather than normal output. Helps if stdout is piped elsewhere.Suggested fix
- fmt.Printf("invalid log level: %s, defaulting to info\n", logFlags.logLevel) + fmt.Fprintf(os.Stderr, "invalid log level: %s, defaulting to info\n", logFlags.logLevel)And add
"os"to imports.
178-183: Slice aliasing may cause issues if handlers are derived concurrently.When copying the handler,
newHandler.attrsshares the backing array withh.attrs. If there is spare capacity,appendwon't allocate, and concurrentWithAttrscalls could step on each other.Force a fresh slice to avoid this:
Suggested fix
func (h *GeneratorLogger) WithAttrs(attrs []slog.Attr) slog.Handler { newHandler := *h - newHandler.attrs = append(newHandler.attrs, attrs...) + newHandler.attrs = make([]slog.Attr, len(h.attrs), len(h.attrs)+len(attrs)) + copy(newHandler.attrs, h.attrs) + newHandler.attrs = append(newHandler.attrs, attrs...) return &newHandler }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/logger.go
🔇 Additional comments (4)
map-generator/logger.go (4)
77-96: Clean constructor with good defensive coding.The nil check for
opts.Levelat lines 92-94 prevents potential nil pointer issues. Sharing the mutex pointer across derived handlers is the right approach for thread-safe writes.
106-176: Handle method looks correct; past review issues resolved.The operator precedence fix on line 156 with proper parentheses is in place. The group prefix is now written at lines 166-168. Filter logic for performance/removal flags and test maps is clear and works as intended.
185-197: WithGroup implementation is correct.The empty name guard at line 188-190 prevents unnecessary copies. String concatenation for the prefix is safe since Go strings are immutable.
199-213: Context utilities follow standard Go patterns.Using an unexported empty struct as the context key avoids collisions. Falling back to
slog.Default()when no logger is found is a sensible default.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
map-generator/map_generator.go (2)
174-183: Good validation; enhance error message with map name.The zero land tile check now returns an error, which correctly addresses the previous review comment. The debug logs and performance warning provide useful observability.
Consider including the map name in the error message for easier debugging when processing multiple maps:
🔎 Suggested improvement
if mapNumLandTiles == 0 { - return MapResult{}, fmt.Errorf("Map has 0 land tiles") + return MapResult{}, fmt.Errorf("map %q has 0 land tiles", args.Name) }
550-584: Context propagation complete; consider logBinaryAsBits verbosity.The context threading through
packTerrain,createMapThumbnail, andlogBinaryAsBitsis consistent.The call to
logBinaryAsBitsat line 582 always logs binary representation of packed data. This seems like debugging detail that might clutter logs during normal operation, especially when processing multiple maps. Consider making this conditional (e.g., only at ALL log level) or removing if it's not needed for operational visibility.Also applies to: 589-616, 694-705
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/map_generator.go
🔇 Additional comments (6)
map-generator/map_generator.go (6)
15-24: Good documentation of constraints.The updated comments and new recommended size constants improve code clarity. The values provide clear boundaries for performance validation.
267-301: Context integration is clean.The function correctly accepts context and uses it for logging. The logic remains focused on shoreline identification.
306-350: Logging integration maintains algorithm clarity.Context and logging added without complicating the BFS distance calculation logic.
389-460: Well-structured logging for water body processing.The logging provides good visibility into water body identification, removal, and processing. The removal log at line 443 includes helpful details (coordinates, size) while using the appropriate tag for filtering.
493-540: Consistent logging pattern for island removal.Matches the water body removal pattern effectively. The debug log provides details while the info log gives a summary.
102-121: Context threading and logger handling are correctly implemented.The context parameter and logger retrieval follow Go patterns well. LoggerFromContext (map-generator/logger.go:203-208) safely handles missing loggers by returning
slog.Default()as a graceful fallback. The type assertion with theokcheck prevents nil returns and panics. The area validation logic correctly identifies maps outside the recommended range.No action needed—the logger availability pattern is robust throughout the file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
map-generator/map_generator.go (2)
116-116: Consider using structured logging attributes.The code uses
fmt.Sprintfto build log messages. While functional, slog supports structured key-value pairs that are more machine-parseable and don't require string formatting:logger.Info("Processing Map", "name", args.Name, "width", width, "height", height) area := width * height if area < minRecommendedPixelSize || area > maxRecommendedPixelSize { logger.Debug("Map area outside recommended range", "area", area, "min", minRecommendedPixelSize, "max", maxRecommendedPixelSize, PerformanceLogTag) }This pattern separates data from message formatting and enables better log querying.
Also applies to: 118-121
102-206: Consider adding context cancellation checks for long-running operations.While context is properly propagated, the code doesn't check for cancellation (
ctx.Err()orctx.Done()). For large maps, generation can take considerable time, and supporting cancellation would improve user experience:// Example in pixel processing loop (lines 130-149) for x := 0; x < width; x++ { if x%100 == 0 { // Check periodically, not every iteration if err := ctx.Err(); err != nil { return MapResult{}, fmt.Errorf("map generation cancelled: %w", err) } } for y := 0; y < height; y++ { // ... existing pixel processing } }Similar checks could be added in BFS loops within
getAreaandprocessDistToLand.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/map_generator.go
🧰 Additional context used
🧬 Code graph analysis (1)
map-generator/map_generator.go (1)
map-generator/logger.go (3)
LoggerFromContext(203-208)PerformanceLogTag(26-26)RemovalLogTag(29-29)
🔇 Additional comments (12)
map-generator/map_generator.go (12)
5-5: LGTM! Clean context integration and well-documented constants.The
contextimport and new performance threshold constants are well-chosen. The recommended ranges (2-3M pixels, max 3M land tiles) provide sensible guardrails for map generation performance.Also applies to: 15-24
102-103: LGTM! Idiomatic context propagation pattern.Accepting
context.Contextas the first parameter and extracting the logger viaLoggerFromContext(ctx)follows Go best practices. This enables per-map contextual logging as intended by the PR objectives.
151-152: LGTM! Consistent context propagation.All helper functions correctly receive the context parameter, maintaining a consistent pattern throughout the map generation pipeline.
Also applies to: 155-155, 158-158, 160-160
170-176: LGTM! Useful land tile count logging.The debug logs for land tile counts at each scale (1x, 4x, 16x) provide valuable diagnostics when running with verbose flags. The context-aware
packTerraincalls maintain consistency.
178-183: LGTM! Critical safeguards properly implemented.The zero land tiles check now returns an error, correctly addressing the past review comment. The performance warning for excessive land tiles provides helpful diagnostics without blocking generation. Both checks are appropriate safeguards.
267-269: LGTM! Context-aware shoreline processing.The function correctly accepts context and uses the extracted logger for informational output.
306-308: LGTM! Context-aware distance calculation.The function signature and logging follow the established pattern correctly.
389-391: LGTM! Comprehensive water body processing with granular logging.The function properly propagates context, uses appropriate log levels (Info for summaries, Debug with
RemovalLogTagfor individual removals), and passes context to nested calls. The logging granularity aligns well with the PR objectives for verbose and removal logging flags.Also applies to: 436-436, 440-443, 451-451, 455-459
493-494: LGTM! Consistent island removal logging.The function follows the same logging pattern as
processWater, using Debug level withRemovalLogTagfor individual removals and Info for summaries. Context handling is correct.Also applies to: 530-530, 539-539
550-550: LGTM! Pack terrain with context-aware logging.The function signature correctly accepts context and propagates it to
logBinaryAsBits. The addition ofnumLandTilesas a return value supports the new validation checks inGenerateMap.Also applies to: 582-582
589-591: LGTM! Context-aware thumbnail creation.The function correctly integrates context and logging following the established pattern.
694-695: LGTM! Binary logging updated for context.The debug helper function correctly accepts context and uses the extracted logger for binary output visualization.
Also applies to: 704-704
iiamlewis
added
the
Maps
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/main.go
🧰 Additional context used
🧬 Code graph analysis (1)
map-generator/main.go (2)
map-generator/logger.go (4)
LogFlags(15-20)ContextWithLogger(211-213)NewGeneratorLogger(79-96)DetermineLogLevel(33-65)map-generator/map_generator.go (2)
GenerateMap(102-206)GeneratorArgs(73-77)
🔇 Additional comments (6)
map-generator/main.go (6)
4-4: LGTM! Appropriate imports for context propagation and structured logging.The addition of
contextandlog/slogpackages aligns with the PR objectives to implement context-aware, structured logging using Go's slog package.Also applies to: 9-9
70-74: LGTM! Standard Go flag pattern with clear documentation.The package-level flag variables follow Go conventions for flag parsing and are well-documented.
105-105: LGTM! Proper context propagation following Go idioms.The context parameter is correctly added as the first parameter to
processMapand properly threaded through toGenerateMap.Also applies to: 136-136
233-235: LGTM! Proper per-map logger and context wiring.Each goroutine correctly creates a per-map logger derived from the default logger with map-specific attributes, wraps it in a context, and passes it through to
processMap. This enables map-specific logging as intended.
258-264: LGTM! Flag definitions match PR objectives and follow Go conventions.The command-line flags are properly defined with clear help text, and the
-vshorthand correctly aliases-verboseusing the standard Go flag pattern.
266-274: LGTM! Proper slog logger initialization with custom handler.The logger is correctly initialized with the custom
GeneratorLoggerhandler, appropriate options, and log level determined from flags. Setting it as the default logger enables per-map loggers to derive from it.
update var naming to go conventions Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @map-generator/main.go:
- Around line 236-241: Remove the duplicate logger declaration and the invalid
references to MapLogTag/TestLogTag: keep the initial creation using mapLogTag
and testLogTag (mapLogTag := slog.String("map", mapItem.Name); testLogTag :=
slog.Bool("isTest", mapItem.IsTest); logger :=
slog.Default().With(mapLogTag).With(testLogTag)), delete the subsequent line
that re-declares logger with MapLogTag/TestLogTag, and ensure
ContextWithLogger(ctx, logger) and the call to processMap(ctx, mapItem.Name,
mapItem.IsTest) use the single valid logger variable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/main.go
🧰 Additional context used
🧬 Code graph analysis (1)
map-generator/main.go (2)
map-generator/logger.go (4)
LogFlags(15-20)ContextWithLogger(211-213)NewGeneratorLogger(79-96)DetermineLogLevel(33-65)map-generator/map_generator.go (2)
GenerateMap(102-206)GeneratorArgs(73-77)
🔇 Additional comments (6)
map-generator/main.go (6)
4-4: LGTM! Imports are appropriate.The
contextandlog/slogimports are correctly added to support context propagation and structured logging throughout the map generator.Also applies to: 9-9
75-79: LGTM! Package variables are well-documented.The package-level variables for flags are appropriately scoped and clearly documented.
110-110: LGTM! Context parameter follows Go conventions.The updated signature correctly adds
context.Contextas the first parameter, following idiomatic Go patterns.
141-145: LGTM! Context propagation is correct.The context is properly threaded through to
GenerateMap, enabling per-map logging throughout the generation pipeline.
264-270: LGTM! Flag definitions are clear and comprehensive.The new logging flags are well-documented and provide the granular control described in the PR objectives. Both long and short forms for the verbose flag improve usability.
272-280: LGTM! Logger initialization is properly structured.The logger is correctly initialized with the custom handler and set as the default, enabling flag-driven logging behavior throughout the application.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
map-generator/main.go (1)
236-240: Consider combining the.With()calls for slightly cleaner code.The current implementation is correct, but the two
.With()calls on line 238 can be combined into a single call for brevity.♻️ Optional refactor
- mapLogTag := slog.String("map", mapItem.Name) - testLogTag := slog.Bool("isTest", mapItem.IsTest) - logger := slog.Default().With(mapLogTag).With(testLogTag) + logger := slog.Default().With( + slog.String("map", mapItem.Name), + slog.Bool("isTest", mapItem.IsTest), + ) ctx := ContextWithLogger(context.Background(), logger)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/main.go
🧰 Additional context used
🧬 Code graph analysis (1)
map-generator/main.go (2)
map-generator/logger.go (4)
LogFlags(15-20)ContextWithLogger(211-213)NewGeneratorLogger(79-96)DetermineLogLevel(33-65)map-generator/map_generator.go (2)
GenerateMap(102-206)GeneratorArgs(73-77)
🔇 Additional comments (7)
map-generator/main.go (7)
4-4: LGTM: Necessary imports for context and logging.The new imports support context propagation and the slog-based logging system.
Also applies to: 9-9
75-79: LGTM: Standard flag variable declarations.Using module-level variables for CLI flags is a standard Go pattern with the
flagpackage, and this is acceptable for a CLI tool's main package.
110-110: LGTM: Context parameter addition follows Go conventions.Adding
context.Contextas the first parameter enables context propagation and is idiomatic Go.
141-145: LGTM: Context correctly passed to GenerateMap.The context is properly propagated from
processMaptoGenerateMap, enabling downstream functions to access the contextual logger.
264-268: LGTM: CLI flags correctly bound.All logging flags are properly bound to the
logFlagsstruct, and the-vshorthand correctly points to the same variable as-verbose.
271-279: LGTM: Logger initialization is well-structured.The centralized logger is properly configured with the custom handler and log level, then set as the default for access by concurrent goroutines.
285-285: LGTM: Clear success message.The success message provides clear feedback that all maps were generated successfully.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
map-generator/main.go (2)
237-241: Effective per-map logger implementation.The per-map logger correctly attaches structured metadata (map name and test flag) and propagates it through the context. Each goroutine gets isolated logging context.
Minor efficiency improvement: combine .With() calls
- mapLogTag := slog.String("map", mapItem.Name) - testLogTag := slog.Bool("isTest", mapItem.IsTest) - logger := slog.Default().With(mapLogTag).With(testLogTag) + logger := slog.Default().With( + slog.String("map", mapItem.Name), + slog.Bool("isTest", mapItem.IsTest), + )This avoids creating an intermediate logger and is slightly more efficient.
272-287: Logger initialization looks solid.The logger is properly configured with flag-based log levels and set as the default before concurrent map processing begins. The success message (line 286) appropriately uses
fmt.Printlnto ensure visibility regardless of log level.Consider using slog for the error case for consistency
Line 283 uses the old
log.Fatalf, while the rest of the application uses structured logging. For consistency:if err := loadTerrainMaps(); err != nil { - log.Fatalf("Error generating terrain maps: %v", err) + slog.Error("Error generating terrain maps", "error", err) + os.Exit(1) }This uses structured logging and makes the error value easily parseable by log aggregators.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/main.go
🧰 Additional context used
🧬 Code graph analysis (1)
map-generator/main.go (2)
map-generator/logger.go (4)
LogFlags(15-20)ContextWithLogger(211-213)NewGeneratorLogger(79-96)DetermineLogLevel(33-65)map-generator/map_generator.go (2)
GenerateMap(102-206)GeneratorArgs(73-77)
🔇 Additional comments (5)
map-generator/main.go (5)
4-4: LGTM! Necessary imports for context-based logging.The
contextandlog/slogimports are correctly added to support the new structured logging functionality.Also applies to: 9-9
76-80: LGTM! Clean global variable declarations.The global variables for flag storage are well-documented and follow the existing pattern used for
mapsFlag.
111-111: LGTM! Idiomatic context parameter addition.The context parameter follows Go conventions (context as first parameter) and enables proper logging context propagation.
142-142: LGTM! Context properly passed to GenerateMap.The context is correctly threaded through to
GenerateMap, enabling structured logging within map generation.
264-270: LGTM! CLI flags correctly defined.All logging flags are properly bound to the
logFlagsstruct and include clear descriptions. The-vshorthand correctly aliases--verbose.
3 participants
Resolves #2718
Description:
Adds go-style error log levels, with an additional ALL log level.
In addition
-verbose(-v),-log-level,-log-removal, and-log-performanceflags to map generatorgo run .without the new flagsDefault run (matches existing)
go run . >> output.txt 2>&1output.txt
Default run w/
-verbose(log level DEBUG)go run . -v >> output.txt 2>&1output.txt
Default run w/
-log-performancego run . -log-performance >> output.txt 2>&1output.txt
Run of just africa w/ all new logging enabled
go run . -maps=africa -log-level=all >> output.txt 2>&1output.txt
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tidwell