fix: ensure that strings that purely contain integers or floats (e.g. "42") remain strings when parsing profiles#4119
Conversation
…) remain strings when parsing profiles
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a helper that converts numeric-like strings to repr(...) when serializing profile values, applied it to one- and two-level CLI-style formatting, and added an unconditional debug print of the serialized result and parsed YAML. Also added a unit test verifying parsing and evaluation of a numeric-like profile value. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/snakemake/profiles.py`:
- Around line 28-35: The serializer is quoting strings with repr() in
format_val, which changes semantics; change format_val to return string values
unchanged (do not call repr) and ensure format_one_level_dict (and any other
places that call format_val) emit raw string content (e.g., key=val) so
numeric-like strings are preserved without added quotes; update format_val to
only format non-string types as needed and remove repr usage in both format_val
and the similar call at the other occurrence referenced (line 43).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 632f6cc0-23ca-48da-b916-6725aa0b0d76
📒 Files selected for processing (1)
src/snakemake/profiles.py
| def format_val(val): | ||
| if isinstance(val, str): | ||
| return repr(val) | ||
| else: | ||
| return val | ||
|
|
||
| def format_one_level_dict(d): | ||
| return [f"{key}={val}" for key, val in d.items()] | ||
| return [f"{key}={format_val(val)}" for key, val in d.items()] |
There was a problem hiding this comment.
repr() here changes value semantics and breaks profile parsing.
At Line 30, repr(val) adds literal quotes to string values. At Line 35 and Line 43 this emits entries like a='grp1' / rule:key='val' instead of a=grp1 / rule:key=val, which alters parsed values and can break existing behavior (notably groups/group-components style mappings).
Please avoid repr-based quoting in this serializer path; keep raw string content and handle numeric-string preservation where type coercion actually happens.
Suggested minimal correction
- def format_val(val):
- if isinstance(val, str):
- return repr(val)
- else:
- return val
+ def format_val(val):
+ return valAlso applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/snakemake/profiles.py` around lines 28 - 35, The serializer is quoting
strings with repr() in format_val, which changes semantics; change format_val to
return string values unchanged (do not call repr) and ensure
format_one_level_dict (and any other places that call format_val) emit raw
string content (e.g., key=val) so numeric-like strings are preserved without
added quotes; update format_val to only format non-string types as needed and
remove repr usage in both format_val and the similar call at the other
occurrence referenced (line 43).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/snakemake/profiles.py`:
- Line 87: Remove the stray debug print call print(result, parsed_obj) so it
does not emit to stdout on every CLI invocation; if you need the information for
debugging retain it via a logger (e.g., logger.debug("profile parse result=%s
parsed_obj=%s", result, parsed_obj)) in the same function that produces result
and parsed_obj so normal runs remain silent and downstream parsers aren’t
broken.
- Line 9: Replace the plain-string int_re with a raw regex that matches integers
or floats (e.g. change int_re = re.compile("^-?\d+$") to a raw pattern such as
numeric_re = re.compile(r"^-?\d+(\.\d+)?$") and then update the reference in
format_val to use numeric_re; this ensures \d is handled correctly and values
like "3.14" and "-0.5" are recognized as numeric by the existing logic that
preserves numbers-as-strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47bb670b-ea9f-460d-b077-c0a8df2fcb7f
📒 Files selected for processing (1)
src/snakemake/profiles.py
|
|
||
| result[key] = value | ||
|
|
||
| print(result, parsed_obj) |
There was a problem hiding this comment.
Remove debug print statement before merging.
This print(result, parsed_obj) will execute on every CLI invocation that loads a profile, producing unintended output to stdout during normal operation. This can break downstream tooling that parses Snakemake output and degrades user experience.
Proposed fix
- print(result, parsed_obj)
return result📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print(result, parsed_obj) | |
| return result |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/snakemake/profiles.py` at line 87, Remove the stray debug print call
print(result, parsed_obj) so it does not emit to stdout on every CLI invocation;
if you need the information for debugging retain it via a logger (e.g.,
logger.debug("profile parse result=%s parsed_obj=%s", result, parsed_obj)) in
the same function that produces result and parsed_obj so normal runs remain
silent and downstream parsers aren’t broken.
This does appear to fix the issue when we explicitly quote the partition name. |
|
Thanks for your help @andynu! |
🤖 I have created a release *beep* *boop* --- ## [9.17.3](v9.17.2...v9.17.3) (2026-03-24) ### Bug Fixes * add curl when containerize with wrapper ([#4115](#4115)) ([44979e4](44979e4)) * ensure proper wrapper prefix is passed to CWL and shown in wrapper error messages ([#4121](#4121)) ([11b6f29](11b6f29)) * ensure that strings that purely contain integers or floats (e.g. "42") remain strings when parsing profiles ([#4119](#4119)) ([3ca08e1](3ca08e1)) * incorrect highlighting in HTML report ([#4120](#4120)) ([1ef224d](1ef224d)) ### Documentation * document an accidental (sorry) recent breaking change in type checking compatibility of Python scripts, in favor of a clean and robust new syntax ([#4116](#4116)) ([013bc43](013bc43)) * Rework tutorial ([#4068](#4068)) ([4bba4a9](4bba4a9)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Released version 9.17.3 <!-- end of auto-generated comment: release notes by coderabbit.ai -->
fixes #3992
QC
docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).AI-assistance disclosure
I used AI assistance for:
Summary by CodeRabbit