fix: handle integer slurm_account values from YAML parsing

[andchamorro]

Summary

Snakemake's YAML parser automatically converts numeric-looking strings (e.g., "123456789") to integers when populating job.resources. This caused account.lower() in test_account() to fail since int has no lower() method.

Changes

Convert slurm_account values to strings before use in re.split() to handle both string and integer values.

Tests

Added 3 new unit tests covering:

• String account values
• Integer account values (from YAML parsing)
• Multiple string accounts

Summary by CodeRabbit

• Bug Fixes

  • Normalize SLURM account inputs (including numeric values) to strings, split multiple accounts, and ensure submission arguments are consistently formatted and quoted.

• Tests

  • Added unit tests validating single, numeric, and multi-account scenarios to ensure correct account argument generation and invocation.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Rewrite of small internals and tests: tuple unpacking style and warning-string formatting in check_active_jobs, coercion of job.resources.slurm_account to str before splitting, and added unit tests for account parsing and -A argument emission.

Changes

Cohort / File(s)	Summary
Executor internals snakemake_executor_plugin_slurm/__init__.py	Changed tuple assignment to explicit unpacking in check_active_jobs; reformatted a multi-line PREEMPTED warning string; coerce job.resources.slurm_account with str(...) before re.split(...) so account tokens are strings passed to shlex.quote.
Account argument tests tests/test_account.py	Add tests for Executor.get_account_arg() validating: string account, integer account normalized to string, and multi-account tokenization (commas/spaces); assert produced -A <account> args and test_account() invocation per token.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 I hopped through code with whiskers bright,
Turned numbers to strings by soft moonlight.
Accounts split tidy, one flag per name,
Tests thump their chests — the rules stay the same. 🥕

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: handling integer slurm_account values that result from YAML parsing, which is the core purpose of this fix.
Docstring Coverage	✅ Passed	Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.

_{✏️ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (2)

tests/test_array_jobs.py (1)

685-692: Test name is misleading: tests single integer, not multiple accounts.

The test name test_multiple_accounts_integer suggests it tests multiple integer accounts, but it only tests a single integer value 123. Consider renaming to test_single_integer_account for clarity, or update the docstring to reflect the actual test purpose.

♻️ Proposed fix for naming clarity

     `@patch`("snakemake_executor_plugin_slurm.get_account")
     `@patch`("snakemake_executor_plugin_slurm.test_account")
-    def test_multiple_accounts_integer(self, mock_test_account, mock_get_account, tmp_path):
-        """Multiple integer accounts work correctly."""
+    def test_single_integer_account(self, mock_test_account, mock_get_account, tmp_path):
+        """Single integer account is converted to string correctly."""
         executor = self._make_executor_stub()
         job = _make_mock_job(slurm_account=123)
         results = list(executor.get_account_arg(job))
         assert results == [" -A 123"]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_array_jobs.py` around lines 685 - 692, The test function name and
docstring are misleading—rename the test function test_multiple_accounts_integer
to test_single_integer_account and update its docstring from "Multiple integer
accounts work correctly." to "Single integer account works correctly." to match
the actual behavior; locate the test by the function name
test_multiple_accounts_integer and references to
_make_mock_job(slurm_account=123) and executor.get_account_arg to make the
change.

snakemake_executor_plugin_slurm/__init__.py (1)

1595-1605: Consider removing duplicate test_account calls.

Each account is validated twice: once in the first loop (lines 1595-1598) and again in the second loop (line 1604). The first validation pass is sufficient.

♻️ Proposed fix to remove duplicate validation

         accounts = [
             a for a in re.split(r"[,\s]+", str(job.resources.slurm_account)) if a
         ]
         for account in accounts:
             # here, we check whether the given or guessed account is valid
             # if not, a WorkflowError is raised
             test_account(account, self.logger)
         # sbatch only allows one account per submission
         # yield one after the other, if multiple were given
         # we have to quote the account, because it might
         # contain build-in shell commands - see issue `#354`
         for account in accounts:
-            test_account(account, self.logger)
             yield f" -A {shlex.quote(str(account))}"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@snakemake_executor_plugin_slurm/__init__.py` around lines 1595 - 1605, The
code calls test_account twice for each entry in accounts — once in the initial
for loop and again inside the yield loop; remove the first validation loop so
each account is validated only once, keeping the validation call in the yield
loop (test_account(account, self.logger)) before yielding f" -A
{shlex.quote(str(account))}" to preserve the behavior and logging; ensure no
other code relies on the removed loop.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@snakemake_executor_plugin_slurm/__init__.py`:
- Around line 1595-1605: The code calls test_account twice for each entry in
accounts — once in the initial for loop and again inside the yield loop; remove
the first validation loop so each account is validated only once, keeping the
validation call in the yield loop (test_account(account, self.logger)) before
yielding f" -A {shlex.quote(str(account))}" to preserve the behavior and
logging; ensure no other code relies on the removed loop.

In `@tests/test_array_jobs.py`:
- Around line 685-692: The test function name and docstring are
misleading—rename the test function test_multiple_accounts_integer to
test_single_integer_account and update its docstring from "Multiple integer
accounts work correctly." to "Single integer account works correctly." to match
the actual behavior; locate the test by the function name
test_multiple_accounts_integer and references to
_make_mock_job(slurm_account=123) and executor.get_account_arg to make the
change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 69d92cfd-ca73-4e22-96ec-16e0dc53400b

📥 Commits

Reviewing files that changed from the base of the PR and between 7fa975f and b9346af.

📒 Files selected for processing (2)

• snakemake_executor_plugin_slurm/__init__.py
• tests/test_array_jobs.py

[cmeesters]

Thank you for your contribution! I left a couple of comments to address. Generally speaking, I am happy to accept.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/test_account.py (1)

47-83: Consider asserting fallback path is not used in explicit-account tests.

Since these tests provide slurm_account, adding mock_get_account.assert_not_called() would make regressions in control flow easier to catch.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_account.py` around lines 47 - 83, For each test that supplies an
explicit slurm_account (test_string_account, test_integer_account,
test_multiple_accounts_string, test_multiple_accounts_integer), assert that the
fallback get_account path was not used by adding
mock_get_account.assert_not_called() after the existing assertions; locate the
checks in the respective test_* functions that call
executor.get_account_arg(job) and insert the
mock_get_account.assert_not_called() immediately after the result assertions to
ensure regressions in control flow are caught.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/test_account.py`:
- Around line 78-83: Replace the single numeric account in
test_multiple_accounts_integer with a comma-delimited numeric string (e.g.,
slurm_account="123,456") so the test actually covers splitting numeric-looking
multi-accounts; then update the assertion to verify get_account_arg(job)
produces entries for both account IDs (check for both "123" and "456" in the
produced argument(s) or match the same multi-account output format used by other
tests), referencing test_multiple_accounts_integer, _make_mock_job, and
get_account_arg to locate the change.

---

Nitpick comments:
In `@tests/test_account.py`:
- Around line 47-83: For each test that supplies an explicit slurm_account
(test_string_account, test_integer_account, test_multiple_accounts_string,
test_multiple_accounts_integer), assert that the fallback get_account path was
not used by adding mock_get_account.assert_not_called() after the existing
assertions; locate the checks in the respective test_* functions that call
executor.get_account_arg(job) and insert the
mock_get_account.assert_not_called() immediately after the result assertions to
ensure regressions in control flow are caught.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 78910a52-6b68-4be9-8650-8f4ce4aa8972

📥 Commits

Reviewing files that changed from the base of the PR and between b9346af and 7b06a22.

📒 Files selected for processing (2)

• snakemake_executor_plugin_slurm/__init__.py
• tests/test_account.py

✅ Files skipped from review due to trivial changes (1)

• snakemake_executor_plugin_slurm/init.py

[cmeesters]

@andchamorro I was on a holiday after Easter. Actually, I think, the PR is ready to be merged. Please correct the formatting issue (Just run black on the code.)

[andchamorro]

I hope you had a wonderful holidays. I've already made the necessary formatting changes. Thanks for everything.