fix: Wrong linenumbers reported when linting#2985
fix: Wrong linenumbers reported when linting#2985johanneskoester merged 11 commits intosnakemake:mainfrom
Conversation
the workflow object wasn't stored, so its linemaps couldn't be applied
WalkthroughThe recent changes enhance the Snakemake codebase by improving the linting functionality, refining error handling for workflows, and introducing a new test configuration. Key modifications include better line number accuracy in error messages and the ability to store workflow objects internally, which optimizes subsequent operations. These updates aim to address reported issues, particularly incorrect error line reporting, leading to a more robust and maintainable system. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (6)
- snakemake/api.py (1 hunks)
- snakemake/linting/rules.py (1 hunks)
- tests/test_issue2574/Snakefile (1 hunks)
- tests/test_issue2574/config.yaml (1 hunks)
- tests/test_linting.py (1 hunks)
- tests/tests.py (1 hunks)
Additional context used
Ruff
tests/test_linting.py
48-48: Found assertion on exception
einexceptblock, usepytest.raises()instead(PT017)
snakemake/linting/rules.py
14-14: Missing return type annotation for public function
item_desc_json(ANN201)
14-14: Missing type annotation for
selfin method(ANN101)
14-14: Missing type annotation for function argument
rule(ANN001)
18-18: Missing return type annotation for public function
get_lineno(ANN201)
18-18: Missing type annotation for
selfin method(ANN101)
18-18: Missing type annotation for function argument
rule(ANN001)
tests/tests.py
2051-2051: Missing return type annotation for public function
test_issue2574Add return type annotation:
None(ANN201)
2054-2054: Function call with
shell=Trueparameter identified, security issue(S604)
2054-2054: Undefined name
subprocess(F821)
2055-2055: Undefined name
subprocess(F821)
2055-2055: Undefined name
subprocess(F821)
Additional comments not posted (5)
tests/test_issue2574/config.yaml (1)
1-2: Configuration file looks good.The configuration file is minimal and appropriate for testing purposes.
tests/test_issue2574/Snakefile (1)
1-4: Snakefile setup is appropriate for testing.The Snakefile is set up to intentionally access a non-existent key, which is suitable for testing error line number reporting.
snakemake/linting/rules.py (2)
11-12: Centralized line number retrieval is a good practice.The use of
get_linenocentralizes the logic for line number retrieval, enhancing maintainability.
15-16: Consistent use ofget_linenoenhances maintainability.The method's update to use
get_linenoaligns with best practices for centralized logic.snakemake/api.py (1)
354-354: Storing the workflow object improves efficiency.The addition of
self._workflow_store = workflowin thelintmethod enhances efficiency by avoiding redundant calls to_get_workflow.
johanneskoester
left a comment
There was a problem hiding this comment.
Thanks a lot! Can you address the AI review comments? Also, the code should be formatted with black.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
tests/test_issue2574/Snakefile (1)
4-4: Config key does not exist, but variable "X" is used.The config key "unexisting_key" does not exist in the configuration file, confirming the intention to test error handling. However, the variable "X" is used in the Snakefile, contradicting the initial assumption that it was unused. Please review the usage of "X" to ensure it aligns with the intended test logic.
Analysis chain
Verify the non-existence of the config key and the unused variable.
The code segment is accessing a non-existing config key "unexisting_key" and assigning it to an unused variable "X".
Verify the non-existence of the config key:
Verify the unused variable:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the non-existence of the config key "unexisting_key". # Test: Check if the config key exists. Expect: The config key does not exist. if grep -q "unexisting_key" "tests/test_issue2574/config.yaml"; then echo "The config key exists." else echo "The config key does not exist." fiLength of output: 128
Script:
#!/bin/bash # Description: Verify the unused variable "X". # Test: Check if the variable is used. Expect: The variable is not used. if grep -q "X" "tests/test_issue2574/Snakefile"; then echo "The variable is used." else echo "The variable is not used." fiLength of output: 95
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- snakemake/linting/rules.py (1 hunks)
- tests/test_issue2574/Snakefile (1 hunks)
- tests/test_linting.py (1 hunks)
- tests/tests.py (1 hunks)
Additional context used
Path-based instructions (3)
tests/test_linting.py (1)
Pattern
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.snakemake/linting/rules.py (1)
Pattern
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.tests/tests.py (1)
Pattern
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Learnings (1)
tests/tests.py (1)
Learnt from: johanneskoester PR: snakemake/snakemake#2985 File: tests/tests.py:2051-2051 Timestamp: 2024-08-13T09:25:24.046Z Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Ruff
tests/tests.py
2054-2054: Undefined name
subprocess(F821)
2057-2057: Undefined name
subprocess(F821)
2058-2058: Undefined name
subprocess(F821)
Additional comments not posted (7)
tests/test_issue2574/Snakefile (2)
1-1: Verify the existence of the config file.Ensure that the config file "config.yaml" exists in the same directory as this Snakefile.
Run the following script to verify the existence of the config file:
Verification successful
Config file exists as expected.
The config file "config.yaml" is present in the directory "tests/test_issue2574", confirming that the Snakemake file's reference is valid. No issues found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the config file "config.yaml". # Test: Check if the config file exists. Expect: The config file exists. if [ -f "tests/test_issue2574/config.yaml" ]; then echo "The config file exists." else echo "The config file does not exist." fiLength of output: 102
6-6: Verify the purpose of the print statement.The code segment is printing the number 1 without any specific purpose.
Verify the purpose of the print statement:
Verification successful
The print statement appears to be used for debugging purposes.
The
print(1)statement follows an attempt to access a non-existing key in a configuration file, suggesting it might be used to confirm that the script reaches this point despite the error. There is no indication of any other specific purpose for this print statement.
- Location:
tests/test_issue2574/Snakefile, line 6Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the purpose of the print statement. # Test: Check if the print statement is used for any specific purpose. Expect: The print statement is not used for any specific purpose. if grep -q "print(1)" "tests/test_issue2574/Snakefile"; then echo "The print statement is used for a specific purpose." else echo "The print statement is not used for any specific purpose." fiLength of output: 164
Script:
#!/bin/bash # Extract lines around the print statement to understand its context. grep -A 5 -B 5 "print(1)" tests/test_issue2574/SnakefileLength of output: 125
tests/test_linting.py (1)
46-48: LGTM!The code changes are approved. The assertion correctly checks that the output contains the expected line number for the specific linting case.
snakemake/linting/rules.py (4)
7-7: LGTM!The import statement is correct and necessary for using the
Ruletype annotation in theget_linenomethod.
12-13: LGTM!The change to call
self.get_lineno(rule)instead of directly accessingrule.linenois correct and improves the accuracy of line number reporting by allowing for potential adjustments based on the workflow's context.
16-17: LGTM!The change to call
self.get_lineno(rule)instead of directly accessingrule.linenois correct and improves the accuracy of line number reporting by allowing for potential adjustments based on the workflow's context.
19-23: LGTM!The new
get_linenomethod is correctly implemented and improves the accuracy of line number reporting by allowing for potential adjustments based on the workflow's context. The type annotations for theruleparameter and return value are also correct and improve code clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- snakemake/linting/rules.py (1 hunks)
Additional context used
Path-based instructions (1)
snakemake/linting/rules.py (1)
Pattern
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Ruff
snakemake/linting/rules.py
6-6:
snakemake.io.is_flaggedimported but unusedRemove unused import:
snakemake.io.is_flagged(F401)
Additional comments not posted (5)
snakemake/linting/rules.py (5)
4-4: LGTM!The import statement for
Unionfrom thetypingmodule is necessary for using theUniontype in the type annotation of the newget_linenomethod.
8-8: LGTM!The import statement for
Rulefrom thesnakemake.rulesmodule is necessary for using theRuletype in the type annotation of theruleparameter in the newget_linenomethod.
13-14: LGTM!The change to call the new
get_linenomethod in theitem_desc_plainmethod improves the accuracy of line number reporting by allowing for potential adjustments based on the workflow's context.
17-18: LGTM!The change to call the new
get_linenomethod in theitem_desc_jsonmethod improves the accuracy of line number reporting by allowing for potential adjustments based on the workflow's context.
20-24: LGTM!The new
get_linenomethod encapsulates the logic for line number retrieval within its own method, improving the maintainability and clarity of the code. The type annotations for theruleparameter and the return type enhance the code clarity and maintainability.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- snakemake/linting/rules.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- snakemake/linting/rules.py
🤖 I have created a release *beep* *boop* --- ## [8.19.1](v8.19.0...v8.19.1) (2024-09-04) ### Bug Fixes * fix issues with misinterpretation of max-jobs-per-timespan and max-jobs-per-seconds ([#3067](#3067)) ([d82453b](d82453b)) * pip deployment path ([#3062](#3062)) ([bf9305b](bf9305b)) * return empty set if rate limiter at max ([#3060](#3060)) ([4e59963](4e59963)) * use wrapt_timeout_decorator, instead of stopit ([#2938](#2938)) ([3b64e41](3b64e41)) * Wrong linenumbers reported when linting ([#2985](#2985)) ([3a8bd36](3a8bd36)) ### Documentation * update `doc-environment.yml` file and Documentation Setup documentation ([#3058](#3058)) ([a540a2e](a540a2e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Johannes Köster <johannes.koester@tu-dortmund.de>
This PR addresses two issues that led to incorrect linenumbers reported when running
snakemake --lint.--lintmode, the stacktrace wasn't cut to remove Snakemake internals and the linenumbers were not adjusted, because the linemap was not available. Fixed by storing the workflow object to make the linemap accessible.snakemake --lint. Fixed by applying the linemap before outputting the lints.I added tests for both cases.
Fixes #2574.
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).Summary by CodeRabbit
New Features
Bug Fixes
Documentation