Skip to content

Fix split delimiter for float nuisances#1247

Open
secholak wants to merge 1 commit intocms-analysis:mainfrom
secholak:main
Open

Fix split delimiter for float nuisances#1247
secholak wants to merge 1 commit intocms-analysis:mainfrom
secholak:main

Conversation

@secholak
Copy link
Copy Markdown

@secholak secholak commented Apr 21, 2026

Bug in the floatNuisances parsing logic.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed parsing of float parameters to correctly recognize comma-delimited nuisance parameters instead of improperly splitting on character "n".

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 21, 2026

📝 Walkthrough

Walkthrough

A delimiter parsing fix in Combine::run changes how --floatParameters nuisance parameter tokens are extracted from the floatNuisances_ string. The split character changes from "n" to ",", altering which tokens are recognized while preserving special "all" handling.

Changes

Cohort / File(s) Summary
Nuisance Parameter Parsing
src/Combine.cc
Changed floatNuisances_ string delimiter from "n" to "," for correct parameter token recognition.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

A rabbit hops through parsing code,
Where "n" once split the heavy load,
Now commas mark the rightful way,
These nuisance tokens bloom and play! 🥕✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately describes the main change: fixing the delimiter used to split float nuisances from 'n' to ','.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/Combine.cc`:
- Around line 634-637: The CI failure is due to formatting changes around the
block handling floatNuisances_ in Combine.cc; run clang-format (or
git-clang-format HEAD~ -- src/) and reformat the block containing the
floatNuisances_ check/branch (the if (floatNuisances_=="all") {
toFloat.add(*nuisances); } else { std::vector<std::string> nuisToFloat =
Utils::split(floatNuisances_, ","); }) so whitespace/brace placement matches
project style, then stage the reformatted changes and push.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e56ecab5-c746-4a0e-93ea-d70e3cb41d50

📥 Commits

Reviewing files that changed from the base of the PR and between 08a3828 and 9d7a870.

📒 Files selected for processing (1)
  • src/Combine.cc

Comment thread src/Combine.cc
Comment on lines 634 to +637
if (floatNuisances_=="all") {
toFloat.add(*nuisances);
} else {
std::vector<std::string> nuisToFloat = Utils::split(floatNuisances_, "n");
std::vector<std::string> nuisToFloat = Utils::split(floatNuisances_, ",");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Run clang-format on the touched block.

The comma delimiter fix on Line 637 looks correct and matches the freeze-parameter parser, but CI is failing with git-clang-format differences around Line 634.

git clang-format HEAD~ -- src/
🧰 Tools
🪛 GitHub Actions: clang-format

[error] 634-634: CI failed due to git-clang-format formatting differences in this file. The pipeline suggests running locally: git-clang-format HEAD~ (command: git clang-format ... -- src/).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Combine.cc` around lines 634 - 637, The CI failure is due to formatting
changes around the block handling floatNuisances_ in Combine.cc; run
clang-format (or git-clang-format HEAD~ -- src/) and reformat the block
containing the floatNuisances_ check/branch (the if (floatNuisances_=="all") {
toFloat.add(*nuisances); } else { std::vector<std::string> nuisToFloat =
Utils::split(floatNuisances_, ","); }) so whitespace/brace placement matches
project style, then stage the reformatted changes and push.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 21, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 20.89%. Comparing base (08a3828) to head (9d7a870).

Files with missing lines Patch % Lines
src/Combine.cc 0.00% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (98.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1247   +/-   ##
=======================================
  Coverage   20.89%   20.89%           
=======================================
  Files         195      195           
  Lines       26310    26310           
  Branches     3947     3947           
=======================================
  Hits         5498     5498           
  Misses      20812    20812           
Files with missing lines Coverage Δ
src/Combine.cc 50.17% <0.00%> (ø)
Files with missing lines Coverage Δ
src/Combine.cc 50.17% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant