-
Notifications
You must be signed in to change notification settings - Fork 762
Add map-generator --maps flag to process a subset of maps #2595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…-generator README.
WalkthroughThe changes introduce a new optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
map-generator/main.go (1)
169-187: Trim whitespace from map names for better user experience.The validation logic is correct, but users might naturally type
--maps=world, africawith spaces after commas. Currently this would fail withmap " africa" is not defined. While debuggable, trimming whitespace is a common courtesy in CLI tools.Apply this diff to handle whitespace gracefully:
func parseMapsFlag() (map[string]bool, error) { if mapsFlag == "" { return nil, nil } validNames := make(map[string]bool, len(maps)) for _, m := range maps { validNames[m.Name] = true } selected := make(map[string]bool) for _, name := range strings.Split(mapsFlag, ",") { + name = strings.TrimSpace(name) + if name == "" { + continue + } if !validNames[name] { return nil, fmt.Errorf("map %q is not defined", name) } selected[name] = true } return selected, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
map-generator/README.md(1 hunks)map-generator/main.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1655
File: resources/maps/giantworldmap/manifest.json:165-174
Timestamp: 2025-07-31T12:03:08.052Z
Learning: In OpenFrontIO draft PRs, flag names in map manifest files may be placeholders that will be updated to match the actual SVG flag file names when the assets are received. The final naming depends on the actual flag SVG files provided, not code naming conventions.
🔇 Additional comments (7)
map-generator/main.go (6)
5-5: LGTM!The standard library imports are appropriate for the new CLI flag functionality.
Also applies to: 10-10
14-14: LGTM!Package-level variable follows standard Go flag package conventions.
190-193: LGTM!The flag parsing and validation is correctly integrated at the start of processing, with proper error propagation.
199-201: LGTM!The filter logic correctly preserves backward compatibility (process all when flag is omitted) while enabling selective processing when the flag is provided.
203-203: LGTM!The loop variable capture is correct and prevents a common Go concurrency issue where goroutines would all reference the same variable.
227-228: LGTM!The flag definition is clear with a helpful usage example, and parsing happens at the correct point before map processing.
map-generator/README.md (1)
20-28: LGTM!The documentation clearly explains the new flag functionality with helpful examples for both single-map and multi-map scenarios. The examples correctly show comma-separated format without spaces.
evanpelle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Resolves #2594
Description:
--mapsflag to the map-generator that allows passing a subset of maps to process.README.mdw/ documentation for using the new flagUsed the go/flag package.
Running without the flag, or with no arguments matches existing behavior of processing all the maps defined in
main.gogo run .go run . --maps=New behavior is triggered to process a subset of maps by passing the
--mapsflag. Multiple maps are defined in a comma-separated listgo run . --maps=world,africaOr a single map (useful for map development)
go run . --maps=fourislandsIf an invalid map is passed, the process aborts and renders the error
By using the default flag package, we get
--helpfor free:I do not write GO often. Gemini contributed to this syntax.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tidwell