snakemake/cli.py (1)

2152-2152: LGTM! Important safety improvement.

The change correctly ensures that --delete-all-output respects the --dry-run flag by passing the dryrun parameter to delete_output(). This is a critical safety improvement that prevents accidental file deletion when dry run mode is enabled.

This change follows the principle of least surprise by making the --dry-run flag behavior consistent across all operations, including destructive ones like --delete-all-output.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

snakemake/ioutils/input.py (1)

12-31: ⚠️ Potential issue

Fix several issues in the extract_checksum function.

The function has a few issues that need to be addressed:

1. The WorkflowError is undefined and needs to be imported.
2. The lambda function should be defined as a regular function per best practices.
3. The exception handling doesn't preserve the traceback.

Apply these changes:

+from snakemake.exceptions import WorkflowError

def parse_input(infile, parser, **kwargs):
    def inner(wildcards, input, output):
        with open(infile, "r") as fh:
            if parser is None:
                return fh.read().strip()
            else:
                return parser(fh, **kwargs)

    return inner


def extract_checksum(infile, **kwargs):
    try:
        import pandas as pd

-        fix_file_name = lambda x: x.removeprefix("./")
+        def fix_file_name(x):
+            return x.removeprefix("./")
        return (
            pd.read_csv(
                infile,
                sep="  ",
                header=None,
                engine="python",
                converters={1: fix_file_name},
            )
            .set_index(1)
            .loc[fix_file_name(kwargs.get("file"))]
            .item()
        )
    except ImportError as err:
-        raise WorkflowError("Pandas is required to extract checksum from file.")
+        raise WorkflowError("Pandas is required to extract checksum from file.") from err

🧰 Tools

🪛 Ruff (0.8.2)

16-16: Do not assign a lambda expression, use a def

Rewrite fix_file_name as a def

(E731)

---

30-30: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

30-30: Undefined name WorkflowError

(F821)

snakemake/deployment/singularity.py (1)

130-130: Removal of --cleanenv flag may affect container environment isolation

The --cleanenv flag has been removed from the singularity command. This flag previously prevented the host environment from being passed to the container (except for explicitly set variables).

Consider documenting this change if it's intentional, as it changes the container isolation behavior. If this change is related to the PR objective of fixing --delete-all-output respecting --dry-run, please add a comment explaining the connection.

snakemake/utils.py (3)

111-117: Ensure clarity on external variables in nested function
The nested function _validate_record(record) relies on set_default, schema, and resolver from the outer scope. While this works due to Python scoping rules, it may be clearer to pass these as parameters. If code maintainability is a concern, refactoring the function signature to accept them directly can improve readability.

---

150-218: Partially validating polars LazyFrame & raising from original exceptions

1. For LazyFrame (lines 189–212), only the first 1000 rows are checked. Consider a configurable chunk to better control validation time for large datasets.
2. As with _validate_pandas, use raise ... from e at lines 205–208 for improved traceback linking:

-except jsonschema.exceptions.ValidationError as e:
-    raise WorkflowError(f"Error validating row {i} of data frame.", e)
+except jsonschema.exceptions.ValidationError as e:
+    raise WorkflowError(f"Error validating row {i} of data frame.") from e

🧰 Tools

🪛 Ruff (0.8.2)

168-170: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

206-208: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

219-230: Streamline exceptions when validating dictionary
At line 224, apply the same practice of raising from the original exception:

-raise WorkflowError("Error validating config file.", e)
+raise WorkflowError("Error validating config file.") from e

🧰 Tools

🪛 Ruff (0.8.2)

224-224: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

docs/snakefiles/deployment.rst (1)

429-429: Minor rewording suggestion
The introductory sentence for container usage could be expanded for clarity, e.g. “As an alternative to using Conda for package management, you can isolate and run entire jobs in containers.”

snakemake/script/__init__.py (1)

56-103: Consider simplifying anchor logic
In the ReportHref class, lines 97–100 could be shortened using the Python inline if-expression, which can be more concise:

-if self._anchor:
-    anchor = f"#{urllib.parse.quote(self._anchor)}"
-else:
-    anchor = ""
+anchor = f"#{urllib.parse.quote(self._anchor)}" if self._anchor else ""

Otherwise, the overall approach for building hierarchical links is well-structured.

🧰 Tools

🪛 Ruff (0.8.2)

97-100: Use ternary operator anchor = f"#{urllib.parse.quote(self._anchor)}" if self._anchor else "" instead of if-else-block

Replace if-else-block with anchor = f"#{urllib.parse.quote(self._anchor)}" if self._anchor else ""

(SIM108)

tests/tests.py (1)

18-18: Remove unused import.

Since patch("snakemake.utils.min_version", ...) references the attribute by string, this local import isn’t used. Please remove it to clean up.

- from snakemake.utils import min_version  # import so we can patch out if needed

🧰 Tools

🪛 Ruff (0.8.2)

18-18: snakemake.utils.min_version imported but unused

Remove unused import: snakemake.utils.min_version

(F401)

misc/vim/syntax/snakemake.vim (1)

4-4: Check the "Last Change" date

The last change date is set to 2025 Mar 6, which is in the future. Consider updating it to the current date.

-" Last Change: 2025 Mar 6
+" Last Change: 2023 Mar 6

snakemake/report/html_reporter/template/components/common.js (1)

19-21: Add missing semicolon for consistency

The new function is missing a semicolon at the end of the return statement, which is inconsistent with the coding style in the rest of the file.

function getResultMimeType(resultPath) {
-    return results[resultPath].mime_type
+    return results[resultPath].mime_type;
}

snakemake/report/html_reporter/template/components/toggle.js (3)

1-1: Remove redundant 'use strict' directive

The 'use strict' directive is redundant in ES modules as they operate in strict mode by default.

-'use strict';

🧰 Tools

🪛 Biome (1.9.4)

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

---

13-13: Remove console.log statement

It's best practice to remove debugging console.log statements before merging to production code.

        // swap the value
-        console.log(selected);
        this.setState({ value: selected });

---

78-110: Consider removing commented-out code

The commented-out render2 method adds unnecessary noise to the codebase. If this alternative rendering approach isn't needed, consider removing it entirely. If it might be needed in the future, add a TODO comment explaining why it's kept.

snakemake/caching/__init__.py (2)

8-18: Consider removing the unused logger import.

Static analysis flags snakemake.logging.logger as unused. If there's no plan to use it, removing it helps keep dependencies clean.

🧰 Tools

🪛 Ruff (0.8.2)

18-18: snakemake.logging.logger imported but unused; consider removing, adding to __all__, or using a redundant alias

(F401)

---

30-30: Chain the original KeyError for clearer debugging.

When re-raising from the KeyError, consider using raise MissingOutputFileCachePathException() from e to preserve traceback context.

🧰 Tools

🪛 Ruff (0.8.2)

30-30: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

tests/test_output_index.py (1)

1-158: Excellent test coverage and clear parameterization.

All tests look functional and robust. You could optionally refactor repeated mocks into a helper fixture for cleaner code and reusability.

tests/test_validate/Snakefile (1)

4-73: Extensive data validation additions look correct.

You've effectively tested with both Pandas and Polars, including LazyFrames. Consider adding a negative test that intentionally violates the schema to confirm proper error handling.

snakemake/workflow.py (3)

127-127: Remove unused import.

The snakemake.api import is not referenced within this file, causing a lint warning. Consider removing it to reduce clutter.

Apply this diff to remove the unused import:

-from snakemake import api, caching, sourcecache
+from snakemake import caching, sourcecache

🧰 Tools

🪛 Ruff (0.8.2)

127-127: snakemake.api imported but unused

Remove unused import: snakemake.api

(F401)

---

256-259: Address the TODO or open a dedicated issue.

There's a TODO comment regarding potential side effects on the source cache. If this needs further investigation, consider opening an issue or providing an explanatory comment for maintainers.

Would you like me to open a new issue or draft a possible fix?

---

1813-1813: Avoid inequality comparison to False.

To improve readability and align with Pythonic style, consider checking truthiness rather than comparing to False.

Apply the following diff to simplify the condition:

- if not ruleinfo.template_engine and ruleinfo.container_img != False:
+ if not ruleinfo.template_engine and ruleinfo.container_img:

🧰 Tools

🪛 Ruff (0.8.2)

1813-1813: Avoid inequality comparisons to False; use if ruleinfo.container_img: for truth checks

Replace with ruleinfo.container_img

(E712)

snakemake/report/html_reporter/template/components/abstract_view_manager.js (1)

1-35: Implement or track the “Unimplemented!” handlers.

The handlers currently throw errors, so they are clearly placeholders. If this is intentional, consider creating tickets or stubs to ensure these methods are fully implemented in a subsequent update.

Would you like me to create an issue for each handler or propose a minimal implementation?

.github/workflows/main.yml (3)

46-54: Keep 'exclude' from being empty

Actionlint warns about an empty exclude block. If it is not needed, consider removing or commenting it out.

-        exclude: []
+        # exclude: []

🧰 Tools

🪛 actionlint (1.7.4)

52-52: "exclude" section should not be empty

(syntax-check)

🪛 YAMLlint (1.35.1)

[error] 53-53: trailing spaces

(trailing-spaces)

---

102-115: Store credentials securely

MinIO credentials are in plain text. Consider using GitHub secrets or environment variables to avoid exposing them.

-        with:
-          access_key: minio
-          secret_key: minio123
+        with:
+          access_key: ${{ secrets.MINIO_ACCESS_KEY }}
+          secret_key: ${{ secrets.MINIO_SECRET_KEY }}

---

211-218: Add a newline at end of file

YAMLlint flags a missing newline at the end of the file (line 218). Please add one to comply with best practices.

+      - run: echo "All tests passed."

+(Add an empty line here)

🧰 Tools

🪛 YAMLlint (1.35.1)

[error] 218-218: no new line character at the end of file

(new-line-at-end-of-file)

snakemake/dag.py (2)

419-424: Consider clarifying multi-line condition

Although valid, you might extract each condition into separate variables for better readability.

-        return (
-            f.is_storage
-            and f
-            not in self.workflow.storage_settings.unneeded_temp_files
-            and await f.exists_local()
-        )
+        is_storage = f.is_storage
+        is_needed_temp = f not in self.workflow.storage_settings.unneeded_temp_files
+        is_local = await f.exists_local()
+        return is_storage and is_needed_temp and is_local

---

427-431: Conditional storing logic

Storing outputs only after the job is finished is sound. Consider separate error-handling paths for logs vs. main output if an exception occurs.

snakemake/report/html_reporter/template/components/abstract_results.js (5)

1-3: Potential delimiter collisions

If array elements contain commas, joining them with “,” could cause collisions in the resulting key. Consider a safer delimiter or an escape mechanism.

---

41-48: Stateful initialization

Storing toggles and data in the component’s state is appropriate. Watch out for large data sets.

---

67-73: Initialize toggles carefully

value[0] might be undefined if ‘values’ is empty. Confirm there's always at least one default entry.

---

109-246: Result table generation

Consider adding accessibility enhancements (captions or ARIA attributes) for better usability.

---

272-320: Row rendering logic

Constructing rows with label values plus action buttons is straightforward. Using a stable key in React for each row is recommended.

snakemake/report/html_reporter/template/components/result_view_button.js (1)

4-70: Consider consolidating repeated logic in the handler methods.

The methods handleImg, handleHtml, and handlePdf share a similar structure, each defining button properties and calling app.setView(...) with only minor differences. You might consider extracting common logic into a helper function that accepts the content type, thus reducing duplication.

snakemake/io.py (4)

686-688: Attach original exception context when re-raising.

Raising:

raise WorkflowError(
    f"Failed to create output directory {dirpath}.", e
)

can lose traceback context. Consider using raise WorkflowError(...) from e to preserve the original exception details, as suggested by static analysis (B904).

🧰 Tools

🪛 Ruff (0.8.2)

686-688: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

693-697: Preserve the original FIFO creation error context.

Similarly, for the FIFO creation error, switch to:

raise WorkflowError(
    f"Failed to create FIFO for pipe output {self._file}.", e
) from e

to retain the underlying exception’s traceback.

🧰 Tools

🪛 Ruff (0.8.2)

695-697: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

945-966: Document new parameter for local consideration.

The _CONSIDER_LOCAL_DEFAULT and the new consider_local parameter in wait_for_files are introduced to selectively skip checking storage for certain files. This is a useful extension, but consider adding a docstring or comment clarifying how and when the user should populate consider_local.

---

1780-1780: Clarify async usage in property.

return async_run(sizes()) within a property might cause confusion since it triggers an async routine in a synchronous context. If this becomes more complex, consider refactoring to a standard async method or clarifying its design so that future maintenance is straightforward.

tests/tests_using_conda.py (1)

61-65: Review or remove the commented-out test portion.

The code around lines 61–65 references a known PermissionError issue with Conda on Windows, commented out for now. If this workaround is no longer needed, consider removing it or marking it clearly with a rationale for future reference.

snakemake/utils.py (1)

118-149: ⚠️ Potential issue

Consider raising from original exception for better traceback
When catching jsonschema.exceptions.ValidationError at lines 131–132 and raising a new WorkflowError, use the raise ... from e form to preserve the original traceback, e.g.:

-except jsonschema.exceptions.ValidationError as e:
-    raise WorkflowError(f"Error validating row {i} of data frame.", e)
+except jsonschema.exceptions.ValidationError as e:
+    raise WorkflowError(f"Error validating row {i} of data frame.") from e

📝 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.

    def _validate_pandas(data):
        try:
            import pandas as pd

            if isinstance(data, pd.DataFrame):
                logger.debug("Validating pandas DataFrame")

                recordlist = []
                for i, record in enumerate(data.to_dict("records")):
                    # Exclude NULL values
                    record = {k: v for k, v in record.items() if pd.notnull(v)}
                    try:
                        recordlist.append(_validate_record(record))
                    except jsonschema.exceptions.ValidationError as e:
                        raise WorkflowError(f"Error validating row {i} of data frame.") from e

                if set_default:
                    newdata = pd.DataFrame(recordlist, data.index)
                    # Add missing columns
                    newcol = newdata.columns[~newdata.columns.isin(data.columns)]
                    data[newcol] = None
                    # Fill in None values with values from newdata
                    data.update(newdata)

            else:
                return False
        except ImportError:
            return False
        return True

🧰 Tools

🪛 Ruff (0.8.2)

132-134: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

snakemake/report/html_reporter/template/components/toggle.js (1)

42-42: ⚠️ Potential issue

Use 'htmlFor' instead of 'for' in React

In React JSX, the HTML attribute 'for' should be written as 'htmlFor'.

-                    {for: valueId(0), className: `${labelClasses} rounded-l`},
+                    {htmlFor: valueId(0), className: `${labelClasses} rounded-l`},

-                    {for: valueId(1), className: `${labelClasses} rounded-r`},
+                    {htmlFor: valueId(1), className: `${labelClasses} rounded-r`},

Also applies to: 59-59

snakemake/jobs.py (1)

1585-1598: 🛠️ Refactor suggestion

Improve exception chaining.

When re-raising an exception in the except ExceptionGroup block, consider Python's recommended practice of using raise ... from e to preserve traceback context.

     except ExceptionGroup as e:
-        raise WorkflowError(
+        raise WorkflowError(
             f"Error postprocessing group job {self.jobid}.",
             *e.exceptions,
+        ) from e
-        )

📝 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.

                async with asyncio.TaskGroup() as tg:
                    for job in level:
                        # postprocessing involves touching output files (to ensure that
                        # modification times are always correct. This has to happen in
                        # topological order, such that they are not mixed up.
                        # We have to disable output mtime checks here
                        # (at least for now), because they interfere with the update of
                        # intermediate file mtimes that might happen in previous levels.
                        tg.create_task(
                            job.postprocess(
                                error=error, check_output_mtime=False, **kwargs
                            )
                        )
            except ExceptionGroup as e:
                raise WorkflowError(
                    f"Error postprocessing group job {self.jobid}.",
                    *e.exceptions,
                ) from e

tests/test_github_issue_3265_respect_dryrun_delete_all/Snakefile (1)

7-19: Consider using rm -f nothere.

If “nothere” is not guaranteed to exist at run-time, rm nothere may fail unexpectedly. Otherwise, this rule successfully tests that --delete-all-output in dry-run mode preserves files.

-        rm nothere && \
+        rm -f nothere && \

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

tests/test_github_issue_3265_respect_dryrun_delete_all/Snakefile_inner (1)

70-76: Missing newline at end of file.

The file is missing a newline character at the end, which is a minor style issue but should be fixed to adhere to common coding standards.

    output:
        touch("temp_keep"),
+

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro