Bump to 1.17.0 and some new i18n patterns#10
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 7 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
Pull Request Overview
The PR updates Opengrep to 1.17.0 and introduces new i18n rules, but several issues should prevent merging in its current state. The static analysis patterns for Python number formatting and C++ std::setprecision are overly broad and will likely cause high false-positive rates in non-user-facing code like logs or data serialization. Additionally, the PR is missing a required test source file (Order.cpp), lacks a description, and contains a potential credential exposure in the Dockerfile. These implementation gaps undermine the reliability of the new rules.
About this PR
- The source file 'Order.cpp' referenced in the expected test results (
results.xml) is missing from the PR diff. This prevents full validation of the new C++ i18n rules. - The Dockerfile contains a redacted secret token ([REDACTED:AWS_SECRET]) in a curl command. This indicates a hardcoded credential pattern in the build process which should be replaced with build secrets or environment variables.
- The PR lacks a description, providing no context for the added i18n rules or the dependency update. Please provide a summary of the changes and the rationale for the specific rule configurations.
Test suggestions
- Detect hardcoded string concatenation in Python print() calls\n- [x] Detect hardcoded date format strings in Python strftime()\n- [x] Detect fixed-point number formatting specifiers in Python f-strings\n- [x] Detect hardcoded string literals in C++ std::cout streams\n- [x] Detect hardcoded date format strings in C++ strftime()\n- [x] Detect usage of std::setprecision for number formatting in C++
🗒️ Improve review quality by adding custom instructions
| severity: WARNING | ||
| languages: | ||
| - python | ||
| pattern-regex: ":\\.[0-9]+f" |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The global regex :\.[0-9]+f is too generic and will flag matches in comments, documentation, or technical strings (like other regexes). Update the rule codacy.python.i18n.no-hardcoded-number-format to use structural Semgrep patterns for f-strings and .format() calls instead of a global pattern-regex.
| current_locale = lang | ||
| _ = locales[lang].gettext | ||
| else: | ||
| print(f"Language {lang} not supported, falling back to English") |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: This fallback error message is hardcoded and won't be translated, which is inconsistent with the i18n practices being established. Since the new 'no-hardcoded-print-concat' rule only flags concatenation, this f-string is missed. Use a translation wrapper: print(_("Language {lang} not supported").format(lang=lang)).
| severity: WARNING | ||
| languages: | ||
| - cpp | ||
| pattern: std::setprecision($N) |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: This rule will flag technical formatting that doesn't require localization (e.g., logging or data serialization). Narrow the codacy.cpp.i18n.no-hardcoded-number-format rule to only flag std::setprecision when it's part of a stream expression involving std::cout or similar user-facing streams.
| confidence: HIGH | ||
| likelihood: HIGH | ||
|
|
||
| - id: codacy.python.i18n.no-hardcoded-number-format |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The rationale for flagging hardcoded number formatting is that decimal separators (period vs. comma) are locale-dependent. Using :.Nf forces a period, which is incorrect for many locales. Consider adding this context to the rule description to help users understand the i18n impact.
There was a problem hiding this comment.
Pull request overview
Updates the docs/test fixtures for i18n rule detection while bumping the bundled Opengrep version to 1.17.0.
Changes:
- Bump Opengrep from 1.16.4 to 1.17.0 (Dockerfile +
.tool_version). - Add Python i18n sample fixture (
Sample.py) to exercise new patterns. - Register new Python/C++ i18n patterns and expected checkstyle results.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/multiple-tests/i18n/src/Sample.py | Adds a Python fixture demonstrating “bad” vs “good” i18n patterns. |
| docs/multiple-tests/i18n/results.xml | Adds expected findings for the new Python and existing C++ fixtures. |
| docs/multiple-tests/i18n/patterns.xml | Enables new Python/C++ i18n rule modules for the multi-test suite. |
| docs/codacy-rules-i18n.yaml | Defines new Semgrep/OpenGrep rules for Python print/strftime/number formatting and C++ cout/strftime/precision patterns. |
| Dockerfile | Bumps default Opengrep release version to v1.17.0. |
| .tool_version | Bumps tool version to 1.17.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # ✅ GOOD: Locale-aware formatting | ||
| locale.setlocale(locale.LC_ALL, current_locale) | ||
| formatted_date = o['date'].strftime(locale.nl_langinfo(locale.D_FMT)) | ||
| formatted_amount = locale.currency(o['amount'], grouping=True) |
There was a problem hiding this comment.
locale.setlocale(locale.LC_ALL, current_locale) is passed values like "en"/"fr", which are not valid locale identifiers on many systems (commonly raises locale.Error: unsupported locale setting). Consider mapping language codes to concrete locales (e.g., "en_US.UTF-8", "fr_FR.UTF-8") and setting the locale once when switching languages (with a safe fallback) rather than inside the per-order loop.
| # ✅ GOOD: Localized message | ||
| print(_("Total Orders: {count}").format(count=len(orders))) | ||
| print(_("Total Revenue: {revenue}").format(revenue=locale.currency(total, grouping=True))) |
There was a problem hiding this comment.
summary() calls locale.currency(total, grouping=True) without ensuring a usable locale has been set. If the process locale is the default "C" locale (or if list_orders() hasn’t run yet / setlocale fails), this can raise ValueError: Currency formatting is not possible using the 'C' locale. Set the locale before currency formatting (or handle failures) to keep this sample runnable.
| @@ -1,4 +1,4 @@ | |||
| ARG OPENGREP_VERSION=v1.16.4 | |||
| ARG OPENGREP_VERSION=v1.17.0 | |||
There was a problem hiding this comment.
CI/README build examples pass --build-arg TOOL_VERSION=$(cat .tool_version), but this Dockerfile only defines/uses OPENGREP_VERSION. As a result, the build arg has no effect and .tool_version can drift from the opengrep binary version. Consider renaming the ARG to TOOL_VERSION (or updating CI/docs to pass OPENGREP_VERSION) and wiring it through consistently.
No description provided.