1.8.5 #228
1.8.5 #228
Conversation
📝 WalkthroughWalkthroughAdds locale-aware capitalization and a hyphen hint, centralizes CLI help-flag handling, introduces two Dagster run-status sensors that POST run results to an n8n webhook (with a secret retriever), updates Definitions to register those sensors, and makes minor workflow and golden-file text updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Dagster as Dagster Pipeline
participant Sensor as RunStatusSensor (discord_*_sensor)
participant Secrets as Secret Retriever
participant n8n as n8n Webhook
Dagster->>Sensor: Emit run status (SUCCESS / FAILURE)
Sensor->>Secrets: fetch_n8n_webhook_secret()
Secrets-->>Sensor: return webhook URL
Sensor->>Sensor: build JSON payload {job_name, status, run_id}
Sensor->>n8n: POST payload (10s timeout)
n8n-->>Sensor: HTTP response / error
Sensor->>Sensor: log response or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
CodSpeed Performance ReportCongrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @card_data/pipelines/sensors.py:
- Line 10: Replace the hardcoded webhook URL string
("https://digitalghost-dev.app.n8n.cloud/...") with an environment variable
N8N_WEBHOOK_URL (use os.getenv or equivalent) in both sensor functions that send
notifications; add a guard at the start of each sensor like: if not
N8N_WEBHOOK_URL: context.log.warning("N8N_WEBHOOK_URL not configured, skipping
notification") and return to skip sending when missing, and use the env var
value when constructing the outgoing request.
In @cmd/card/card.go:
- Around line 29-34: Move the call to utils.CheckHelpFlag(&output, flag.Usage)
to before flag.Parse() so the help branch returns the generated help string
instead of allowing flag.Parse (which may call flag.Usage and exit) to run;
update the control flow in the function that declares output, uses flag.Usage
and calls flag.Parse() so it first checks CheckHelpFlag and returns
output.String() when true, then proceeds to call flag.Parse() and the rest of
the logic; keep using the same output variable and flag.Usage reference and
consider later generalizing utils.CheckHelpFlag to scan all os.Args positions if
needed.
In @flags/pokemonflagset.go:
- Line 231: The change replaced the local formatAbilityName logic with
styling.CapitalizeResourceName, but that utility applies blanket title-casing
and loses the original exception rules (lowercasing "of", "the", "to", "as" when
not the first word); restore the original behavior by either reintroducing the
local formatAbilityName and using it where formattedAbilityName is set, or
modify styling.CapitalizeResourceName to implement the same exception logic
(ensure it lowercases those specific small words unless they are the first
token) and keep the call formattedAbilityName :=
styling.CapitalizeResourceName(abilityName) working identically to the former
local function.
🧹 Nitpick comments (8)
card_data/pipelines/sensors.py (3)
19-20: Improve exception handling.As flagged by static analysis: catching bare
Exceptionis too broad, andlog.errordoesn't include the traceback. Uselog.exceptionfor automatic traceback logging, and consider catchingrequests.RequestExceptionfor more precise error handling.♻️ Proposed fix
- except Exception as e: - context.log.error(f"Failed to send notification: {e}") + except requests.RequestException as e: + context.log.exception(f"Failed to send notification: {e}")
5-39: Consider consolidating duplicate sensor logic.Both sensors share identical logic except for the status value. A helper function would reduce duplication and simplify maintenance.
♻️ Proposed refactor
def _send_notification(context: RunStatusSensorContext, status: str): """Send run notification to n8n webhook.""" webhook_url = os.environ.get("N8N_WEBHOOK_URL") if not webhook_url: context.log.warning("N8N_WEBHOOK_URL not configured") return context.log.info(f"Detected {status.lower()} run: {context.dagster_run.run_id}") try: response = requests.post( webhook_url, json={ "job_name": context.dagster_run.job_name, "status": status, "run_id": context.dagster_run.run_id, }, timeout=10, ) context.log.info(f"n8n response: {response.status_code}") except requests.RequestException as e: context.log.exception(f"Failed to send notification: {e}") @run_status_sensor(run_status=DagsterRunStatus.SUCCESS, name="discord_success_sensor") def discord_success_sensor(context: RunStatusSensorContext): _send_notification(context, "SUCCESS") @run_status_sensor(run_status=DagsterRunStatus.FAILURE, name="discord_failure_sensor") def discord_failure_sensor(context: RunStatusSensorContext): _send_notification(context, "FAILURE")
38-39: Same exception handling issue as the success sensor.Apply the same fix: use
requests.RequestExceptionandlog.exception.flags/pokemonflagset.go (1)
110-131: Remove unused formatAbilityName function.The
formatAbilityNamefunction is no longer called after the refactoring at line 231. This is dead code and should be removed.♻️ Proposed removal of dead code
- // Anonymous function to format ability names - formatAbilityName := func(name string) string { - exceptions := map[string]bool{ - "of": true, - "the": true, - "to": true, - "as": true, - } - - name = strings.ReplaceAll(name, "-", " ") - words := strings.Split(name, " ") - titleCaser := cases.Title(language.English) - - // Process each word - for i, word := range words { - if _, found := exceptions[strings.ToLower(word)]; found && i != 0 { - words[i] = strings.ToLower(word) - } else { - words[i] = titleCaser.String(word) - } - } - return strings.Join(words, " ") - } -cmd/utils/output.go (2)
29-35: Remove unused output parameter.The
output *strings.Builderparameter is declared but never used in the function body. This creates a confusing API surface.♻️ Proposed fix to remove unused parameter
-func CheckHelpFlag(output *strings.Builder, usageFunc func()) bool { +func CheckHelpFlag(usageFunc func()) bool { if len(os.Args) == 3 && (os.Args[2] == "-h" || os.Args[2] == "--help") { usageFunc() return true } return false }Note: This would require updating all call sites (e.g.,
cmd/berry/berry.goline 36 and similar locations in other commands).
30-30: Consider making argument position checking more flexible.The hardcoded check
len(os.Args) == 3andos.Args[2]assumes help flags only appear as the third argument in a specific command structure. This may limit flexibility if the CLI needs to support help flags in different positions or contexts in the future.cmd/move/move.go (1)
15-16: Unused imports after partial refactoring.The
golang.org/x/text/casesandgolang.org/x/text/languageimports remain, used only at lines 83 and 87 for type/category capitalization. Consider usingstyling.CapitalizeResourceNamethroughout for consistency with other commands, which would allow removing these imports.♻️ Suggested refactor
-import ( - "flag" - "fmt" - "os" - "strconv" - "strings" - - "github.com/charmbracelet/lipgloss" - "github.com/digitalghost-dev/poke-cli/cmd/utils" - "github.com/digitalghost-dev/poke-cli/connections" - "github.com/digitalghost-dev/poke-cli/structs" - "github.com/digitalghost-dev/poke-cli/styling" - "golang.org/x/text/cases" - "golang.org/x/text/language" -) +import ( + "flag" + "fmt" + "os" + "strconv" + "strings" + + "github.com/charmbracelet/lipgloss" + "github.com/digitalghost-dev/poke-cli/cmd/utils" + "github.com/digitalghost-dev/poke-cli/connections" + "github.com/digitalghost-dev/poke-cli/structs" + "github.com/digitalghost-dev/poke-cli/styling" +)Then update lines 83 and 87:
- lipgloss.JoinHorizontal(lipgloss.Top, labelStyle.Render("Type"), "|", valueStyle.Render(cases.Title(language.English).String(moveStruct.Type.Name))), + lipgloss.JoinHorizontal(lipgloss.Top, labelStyle.Render("Type"), "|", valueStyle.Render(styling.CapitalizeResourceName(moveStruct.Type.Name))),- lipgloss.JoinHorizontal(lipgloss.Top, labelStyle.Render("Category"), "|", valueStyle.Render(cases.Title(language.English).String(moveStruct.DamageClass.Name))), + lipgloss.JoinHorizontal(lipgloss.Top, labelStyle.Render("Category"), "|", valueStyle.Render(styling.CapitalizeResourceName(moveStruct.DamageClass.Name))),cmd/pokemon/pokemon.go (1)
18-19: Consider removing unused text/cases imports for full consistency.Similar to
move.go, these imports remain forcases.Titleusage at lines 116 and 140 (egg groups and type names). Usingstyling.CapitalizeResourceNamethroughout would allow removing these imports and maintain consistency withability.goanditem.go.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pokemon.svgis excluded by!**/*.svg
📒 Files selected for processing (22)
.github/workflows/go_test.yml.github/workflows/py_test.ymlcard_data/pipelines/definitions.pycard_data/pipelines/sensors.pycli.gocmd/ability/ability.gocmd/berry/berry.gocmd/card/card.gocmd/item/item.gocmd/move/move.gocmd/natures/natures.gocmd/pokemon/pokemon.gocmd/speed/speed.gocmd/types/types.gocmd/utils/output.gocmd/utils/output_test.goflags/pokemonflagset.gostyling/styling.gotestdata/cli_help.goldentestdata/item_help.goldentestdata/main_latest_flag.goldentestdata/move_help.golden
🧰 Additional context used
🧬 Code graph analysis (12)
cmd/utils/output_test.go (1)
cmd/utils/output.go (1)
CheckHelpFlag(29-35)
cmd/types/types.go (1)
cmd/utils/output.go (1)
CheckHelpFlag(29-35)
cmd/berry/berry.go (1)
cmd/utils/output.go (1)
CheckHelpFlag(29-35)
cmd/natures/natures.go (1)
cmd/utils/output.go (1)
CheckHelpFlag(29-35)
flags/pokemonflagset.go (1)
styling/styling.go (1)
CapitalizeResourceName(89-91)
cmd/card/card.go (1)
cmd/utils/output.go (1)
CheckHelpFlag(29-35)
cmd/speed/speed.go (1)
cmd/utils/output.go (1)
CheckHelpFlag(29-35)
cmd/pokemon/pokemon.go (2)
styling/styling.go (3)
StyleItalic(33-33)HyphenHint(30-30)CapitalizeResourceName(89-91)cmd/utils/output.go (1)
CheckHelpFlag(29-35)
cmd/move/move.go (2)
styling/styling.go (3)
StyleItalic(33-33)HyphenHint(30-30)CapitalizeResourceName(89-91)cmd/utils/output.go (1)
CheckHelpFlag(29-35)
cli.go (1)
styling/styling.go (2)
StyleItalic(33-33)HyphenHint(30-30)
card_data/pipelines/definitions.py (1)
card_data/pipelines/sensors.py (2)
discord_success_sensor(6-20)discord_failure_sensor(24-39)
cmd/ability/ability.go (2)
styling/styling.go (3)
StyleItalic(33-33)HyphenHint(30-30)CapitalizeResourceName(89-91)cmd/utils/output.go (1)
CheckHelpFlag(29-35)
🪛 Ruff (0.14.10)
card_data/pipelines/sensors.py
19-19: Do not catch blind exception: Exception
(BLE001)
20-20: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
38-38: Do not catch blind exception: Exception
(BLE001)
39-39: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: tests
- GitHub Check: benchmarks
🔇 Additional comments (22)
testdata/main_latest_flag.golden (1)
5-5: LGTM!Version bump in test golden file aligns with the release progression.
testdata/item_help.golden (1)
5-5: LGTM!Cleaner usage syntax; flags are appropriately documented in the FLAGS section.
testdata/move_help.golden (1)
1-10: LGTM!Help output structure is consistent with
item_help.goldenand follows the standardized format across CLI commands.card_data/pipelines/definitions.py (2)
9-9: LGTM!Import follows the established pattern for local module imports.
31-35: LGTM!Sensors are correctly wired into the Definitions and will be merged with folder definitions via
Definitions.merge..github/workflows/py_test.yml (1)
1-35: LGTM!Well-structured Python test workflow with proper scoping to
card_data/**. The complementary path configuration withgo_test.yml(which usespaths-ignore: card_data/**) ensures each workflow runs only for relevant changes. Action versions are current..github/workflows/go_test.yml (1)
1-19: LGTM!Workflow naming improvements enhance clarity with the new Python Tests workflow. The
paths-ignorecorrectly excludes the Pythoncard_data/directory. Go 1.24 is a stable released version (Feb 11, 2025) and fully supported by setup-go@v5.testdata/cli_help.golden (1)
26-26: Golden help hint update looks consistent—ensure goldens are regenerated from the canonical help renderer.
Ifstyling.HyphenHintchanges again (wording/styling), prefer regenerating goldens via the same code path to avoid brittle manual edits.cmd/natures/natures.go (1)
6-8: Import additions look justified by current usage (os.Args,strings.Builder).cli.go (1)
83-83: LGTM! Centralized help hint improves maintainability.Using the centralized
styling.HyphenHintconstant eliminates hardcoded help text and ensures consistency across the CLI.flags/pokemonflagset.go (1)
468-468: LGTM! Consistent use of centralized capitalization utility.Using
styling.CapitalizeResourceNamefor move names aligns with the refactoring at line 231 for ability names.cmd/utils/output_test.go (1)
140-254: Good test coverage for CheckHelpFlag.The tests comprehensively cover various scenarios including short/long flags, no flag, wrong argument counts, and incorrect flag positioning. The pattern of saving and restoring
os.Argsis appropriate for testing functions that rely on global state.cmd/berry/berry.go (1)
36-38: LGTM! Centralized help flag handling improves consistency.The refactoring to use
utils.CheckHelpFlagaligns with the PR's objective of centralizing help flag detection across CLI commands.styling/styling.go (2)
84-91: Well-implemented centralized capitalization utility.The
titleCaserreuse at package level andCapitalizeResourceNamefunction are clean abstractions. Thecases.Caserfromgolang.org/x/text/casesis safe for concurrent use, so the package-level variable is appropriate.
30-31: Good centralization of the hyphen hint constant.Extracting
HyphenHintas a shared constant eliminates duplication across command files and ensures consistent messaging.cmd/item/item.go (2)
36-44: Consistent adoption of centralized help flag and validation utilities.The refactoring to use
utils.CheckHelpFlagandutils.ValidateItemArgsaligns with the pattern across other commands, improving maintainability.
61-63: Clean usage of centralized capitalization.Using
styling.CapitalizeResourceNamefor both the item name and category ensures consistent formatting and removes the dependency ongolang.org/x/text/casesin this file.cmd/move/move.go (1)
37-45: Centralized help and validation handling looks good.The adoption of
utils.CheckHelpFlagandutils.ValidateMoveArgsaligns with the refactoring pattern across the codebase.cmd/ability/ability.go (2)
38-46: Clean refactoring to centralized utilities.The help flag handling with
utils.CheckHelpFlagand validation withutils.ValidateAbilityArgsare well-integrated. The removal ofgolang.org/x/textimports in favor ofstyling.CapitalizeResourceNameis complete.
82-83: LGTM!Using
styling.CapitalizeResourceNamefor ability name formatting is consistent with the centralized approach.cmd/pokemon/pokemon.go (2)
54-57: Good adoption of centralized help flag handling.The use of
utils.CheckHelpFlagfollows the established pattern across commands.
86-86: Consistent use of CapitalizeResourceName for Pokémon names.Both the main Pokémon name and the evolves-from species name now use the centralized capitalization function, which correctly handles hyphenated names like "mr-mime" → "Mr Mime".
Also applies to: 176-177
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @card_data/pipelines/sensors.py:
- Around line 7-40: Both sensors (discord_success_sensor and
discord_failure_sensor) duplicate POST logic—extract a shared helper (e.g.,
send_n8n_notification) that accepts job_name and status and calls
fetch_n8n_webhook_secret(), posts JSON, calls response.raise_for_status(), and
returns the response; in the sensors call that helper. Tighten error handling to
catch requests.exceptions.RequestException (not Exception) and log with
context.log.exception("Failed to send n8n notification") to avoid embedding the
webhook URL in messages. Optionally rename discord_success_sensor and
discord_failure_sensor to n8n_success_sensor and n8n_failure_sensor to reflect
actual behavior.
In @cmd/ability/ability.go:
- Around line 36-41: The help check in ability.go uses
utils.CheckHelpFlag(&output, flag.Usage) which only inspects the global flag
state and is too strict for forms like "ability <name> -h"; update CheckHelpFlag
to accept an args []string parameter (e.g., func CheckHelpFlag(out
*bytes.Buffer, usage func(), args []string) bool) and change this call in
ability.go to pass the remaining args (e.g., os.Args[1:] or flag.Args()). In the
new CheckHelpFlag implementation scan the provided args for "-h" or "--help" (or
other help tokens) and, if found, write usage to output and return true; keep
existing behavior for the buffer and usage callback otherwise so help works
after the positional ability name.
In @styling/styling.go:
- Around line 7-13: Change the exported mutable variable HyphenHint to an
immutable constant: replace the exported var declaration for HyphenHint with a
const string declaration (preserving the same value and name) so it cannot be
mutated at runtime, and update any references if necessary; apply the same
replacement to the second occurrence noted (the other exported var at the
mentioned location).
🧹 Nitpick comments (1)
card_data/pipelines/utils/secret_retriever.py (1)
26-34: SecretCache/client recreated per call + brittle secret parsing (JSON/key)
- Re-instantiating
SecretCacheevery invocation largely defeats caching and can amplify Secrets Manager traffic.json.loads(secret)+secret_dict["n8n_webhook"]will throw noisyJSONDecodeError/KeyErrorif the secret is stored as a plain string or uses a different key.Proposed refactor (module-level cache + safer decode)
+from typing import NotRequired + +class N8nWebhookSecret(TypedDict): + # If you truly store JSON, prefer a stable key like `url` / `webhook_url`. + n8n_webhook: NotRequired[str] + url: NotRequired[str] + webhook_url: NotRequired[str] + +_secrets_client = botocore.session.get_session().create_client("secretsmanager") +_secrets_cache = SecretCache(config=SecretCacheConfig(), client=_secrets_client) + def fetch_n8n_webhook_secret() -> str: - client = botocore.session.get_session().create_client("secretsmanager") - cache_config = SecretCacheConfig() - cache = SecretCache(config=cache_config, client=client) - - secret = cast(str, cache.get_secret_string("n8n_webhook")) - secret_dict: dict[str, str] = json.loads(secret) - - return secret_dict["n8n_webhook"] + secret = cast(str, _secrets_cache.get_secret_string("n8n_webhook")) + # Allow either a raw URL secret or a JSON object. + try: + secret_dict: N8nWebhookSecret = json.loads(secret) + return ( + secret_dict.get("n8n_webhook") + or secret_dict.get("webhook_url") + or secret_dict.get("url") + or secret # last resort if JSON wasn't the expected shape + ) + except json.JSONDecodeError: + return secretTo validate the expected shape, please confirm what the
"n8n_webhook"secret value looks like in AWS (raw URL string vs JSON, and key name).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
card_data/pipelines/sensors.pycard_data/pipelines/utils/secret_retriever.pycmd/ability/ability.gocmd/ability/ability_test.gocmd/berry/berry.gocmd/card/card.gocmd/item/item.gocmd/move/move.gocmd/natures/natures.gocmd/pokemon/pokemon.gocmd/search/search.gocmd/speed/speed.gocmd/types/types.goflags/pokemonflagset.gostyling/styling.gotestdata/ability-ii.golden
🚧 Files skipped from review as they are similar to previous changes (4)
- flags/pokemonflagset.go
- cmd/pokemon/pokemon.go
- cmd/card/card.go
- cmd/item/item.go
🧰 Additional context used
🧬 Code graph analysis (7)
cmd/ability/ability.go (2)
styling/styling.go (2)
HyphenHint(30-30)CapitalizeResourceName(94-109)cmd/utils/output.go (1)
CheckHelpFlag(29-35)
cmd/types/types.go (1)
cmd/utils/output.go (1)
CheckHelpFlag(29-35)
cmd/ability/ability_test.go (1)
cmd/utils/golden.go (1)
LoadGolden(10-46)
cmd/berry/berry.go (1)
cmd/utils/output.go (1)
CheckHelpFlag(29-35)
cmd/natures/natures.go (1)
cmd/utils/output.go (1)
CheckHelpFlag(29-35)
cmd/speed/speed.go (1)
cmd/utils/output.go (1)
CheckHelpFlag(29-35)
card_data/pipelines/sensors.py (1)
card_data/pipelines/utils/secret_retriever.py (1)
fetch_n8n_webhook_secret(26-34)
🪛 Ruff (0.14.10)
card_data/pipelines/sensors.py
21-21: Do not catch blind exception: Exception
(BLE001)
22-22: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
39-39: Do not catch blind exception: Exception
(BLE001)
40-40: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: tests
- GitHub Check: benchmarks
🔇 Additional comments (13)
card_data/pipelines/sensors.py (1)
1-5: All dependencies have version pins specified in pyproject.tomlThe dependencies referenced in this file (
dagster>=1.11.3,requests>=2.32.4, andaws-secretsmanager-caching>=1.1.3) are all explicitly declared with version constraints inpyproject.toml. Runtime availability is managed through explicit version pinning, ensuring compatible versions across installations.testdata/ability-ii.golden (1)
1-3: LGTM!The test data for the "Beads of Ruin" ability is well-formatted and correctly structured for use in the test case added in
cmd/ability/ability_test.go.cmd/ability/ability_test.go (1)
34-38: LGTM!The new test case for the "beads-of-ruin" ability follows the established pattern and correctly references the corresponding golden file.
cmd/speed/speed.go (1)
108-118: LGTM!The refactoring correctly adopts the centralized help-flag utility and properly sequences
flag.Parse()after help checking. The error handling at lines 115-118 appropriately returns both the accumulated output and the error, maintaining consistent behavior.cmd/types/types.go (1)
31-36: LGTM!The refactoring correctly adopts the centralized help-flag utility and properly sequences
flag.Parse()after help checking, consistent with the broader refactoring pattern across the codebase.cmd/berry/berry.go (1)
33-38: Centralized help handling reads clean here.cmd/natures/natures.go (1)
27-32: Help short-circuit + parse-after-help is consistent.cmd/move/move.go (3)
22-33: Help output formatting is clearer (HyphenHint + FLAGS section).
62-64: Move name capitalization viastyling.CapitalizeResourceNameis a nice reuse.
35-40: Help flag only works before positional arguments, not after.The
CheckHelpFlagfunction checks iflen(os.Args) == 3and the third argument is a help flag. This meanspoke-cli move --helpis supported, butpoke-cli move <name> --helpis not. This is intentional design, as confirmed by the unit tests which only validate the 3-argument case. All commands follow this same pattern consistently.cmd/ability/ability.go (2)
18-28: Help text reuse viastyling.HyphenHintis good.
81-83: Ability name capitalization viastyling.CapitalizeResourceNamelooks correct.styling/styling.go (1)
84-109:CapitalizeResourceName: usestrings.Fieldsto avoid empty tokens; do not extractcaserto package scope.The suggestion to extract
cases.Title(language.English)to a package-level variable would introduce a race condition. Thegolang.org/x/text/cases.Casertype is stateful and explicitly documented as unsafe for concurrent use ("A Caser may be stateful and should therefore not be shared between goroutines"). SinceCapitalizeResourceNameis called from goroutines inflags/pokemonflagset.go, each invocation must create its ownCaserinstance.However, replace
strings.Split(name, " ")withstrings.Fields(name)to automatically handle multiple consecutive spaces and leading/trailing whitespace.Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @styling/styling.go:
- Around line 86-111: CapitalizeResourceName currently creates a new caser with
cases.Title on each call and lowercases non-leading letters, which mangles
acronyms; change to a package-level caser using cases.Title(cases.NoLower()) to
preserve existing capitalization, reuse that caser variable instead of
instantiating inside CapitalizeResourceName, replace strings.Split with
strings.Fields for robust whitespace handling, and keep the smallWords logic but
compare using strings.ToLower(word) before deciding to set words[i] to
strings.ToLower(word) or caser.String(word).
🧹 Nitpick comments (1)
styling/styling.go (1)
15-17:HyphenHintcopy could be a bit more explicit about the literal character (e.g., “Use a hyphen (-) …”).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/codspeed.ymlstyling/styling.go
🔇 Additional comments (3)
.github/workflows/codspeed.yml (2)
3-7: LGTM!The push trigger for the main branch with the
card_data/**path filter is appropriate. This ensures benchmarks run when changes are merged to main, which is useful for tracking performance over time.
34-34: The v4→v7 jump introduces breaking changes: v5 changed cache defaults, v6 removed automatic venv activation (now requiresactivate-environment: true) and removedpyproject-file/uv-fileinputs, and v7 requires Node24 and removed theserver-urlinput. However, this workflow doesn't use any of these affected features—it sets Python viaactions/setup-pythonrather than setup-uv, doesn't configure caching or the removed inputs, and runs on ubuntu-22.04 which supports Node24. The update is safe.styling/styling.go (1)
3-13: Add test coverage forCapitalizeResourceNameto validate expected behavior with edge cases. The function currently lacks test cases, making it difficult to validate expected output for edge cases. Whilecases.Title()can produce unexpected results with acronyms (e.g., "hp-up" → "Hp Up"), and hyphenated canonical names are replaced with spaces, there is no concrete evidence in the codebase that these scenarios occur. The module version is properly pinned tov0.31.0. Consider adding tests that verify expected output for actual data patterns from the API (e.g., ability names, move names) to ensure formatting aligns with user expectations.
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.