Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added documentation and code examples for W0134 return-in-finally checker #1000

Merged
merged 7 commits into from
Jan 28, 2024

Conversation

MariaMa-GitHub
Copy link
Contributor

@MariaMa-GitHub MariaMa-GitHub commented Jan 20, 2024

Motivation and Context

W0134 return-in-finally is a new check in Pylint which was added at the end of last semester, so it needed to be added to the PythonTA documentation. This pull request resolves the issue by providing documentation and examples for this checker.

Fixes #999.

Your Changes

Description:

  • Added documentation for W0134 return-in-finally checker.
  • Wrote a code example that raises the W0134 return-in-finally error.
  • Provided a correction to the example code that demonstrates how to resolve the W0134 return-in-finally error.

Type of change (select all that apply):

  • Documentation update (change that modifies or updates documentation only)

Testing

This pull request was tested using PythonTA with the code below.

import python_ta

python_ta.check_all()

The PythonTA report correctly showed that the W0134 (return-in-finally) error was raised.

Questions and Comments (if applicable)

I am not too sure how to classify the W0134 (return-in-finally) error. I chose to put it under "Improper Python usage" since I think that it does "indicate a misuse of variables, control flow, or other Python features in our code" and it is similar to other errors in the same section (such as Return outside function (E0104) and Unreachable (W0101)). However, when I ran PythonTA, it put the W0134 (return-in-finally) error under "Style and Convention", which also makes a bit of sense. Since I am not too sure how exactly this checker should be classified, I would like to ask under which section should it be put and in the future how should I decide if there are conflicts like this happening.

Checklist

  • I have performed a self-review of my own code.
  • I have verified that the CI tests have passed.
  • I have reviewed the test coverage changes reported on Coveralls.

@coveralls
Copy link
Collaborator

coveralls commented Jan 20, 2024

Pull Request Test Coverage Report for Build 7688896823

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.001%) to 96.891%

Totals Coverage Status
Change from base Build 7393327913: -0.001%
Covered Lines: 3210
Relevant Lines: 3313

💛 - Coveralls

Copy link
Contributor

@david-yz-liu david-yz-liu left a comment

Choose a reason for hiding this comment

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

Good work, @MariaMa-GitHub! I left a few inline comments, and in addition to that have two overall comments:

  1. Since this is your first pull request, please add your name to the list of contributors :)
  2. In terms of decided where to put the error, it's a bit ad hoc right now. One guiding principle I use is to look for similar errors, or errors that are related to the same language features. In this case, you should look for errors related to exceptions (some recent ones are the "too broad exception" ones) and put this entry close to those. Don't worry about categorizing as "style" vs. "error" within PythonTA though, that's something we can revisit later.

docs/checkers/index.md Outdated Show resolved Hide resolved
docs/checkers/index.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/checkers/index.md Outdated Show resolved Hide resolved
docs/checkers/index.md Outdated Show resolved Hide resolved
@david-yz-liu david-yz-liu merged commit a4b0cc5 into pyta-uoft:master Jan 28, 2024
14 checks passed
@MariaMa-GitHub MariaMa-GitHub deleted the document-checker-w0134 branch January 29, 2024 05:08
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.

Add PyTA Documentation for W0134 return-in-finally Pylint Checker
4 participants