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

New configuration option - Allowing "pylint:" commments #973

Merged
merged 20 commits into from
Nov 18, 2023

Conversation

AinaMerch
Copy link
Contributor

Motivation and Context

PythonTA currently looks for a comment that starts with "pylint:" and, if present, does not check the file. I added a new configuration option to let the user customize whether they want to allow or disallow "pylint:" comments. Also add ed documentation for this under the PythonTA configuration page. Added 2 test files alongside.

Your Changes

Added configuration to allow "pylint:" comments through modifying the pylintrc and init.py files. Added documentation to configuration.md.
Description:

Type of change (select all that apply):

  • New feature (non-breaking change which adds functionality)
  • Documentation update (change that modifies or updates documentation only)

Testing

Added 2 test files, allow_pylint_comments_true_testcase.py and allow_pylint_comments_false_testcase.py to check that the config option does what is required.

Questions and Comments (if applicable)

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.
  • I have added tests for my changes.

@coveralls
Copy link
Collaborator

coveralls commented Oct 18, 2023

Pull Request Test Coverage Report for Build 6915840675

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 94.638%

Totals Coverage Status
Change from base Build 6914550247: 0.02%
Covered Lines: 3442
Relevant Lines: 3637

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

@AinaMerch great work, I've left a few comments to address.

python_ta/__init__.py Outdated Show resolved Hide resolved
python_ta/__init__.py Outdated Show resolved Hide resolved
docs/usage/configuration.md Outdated Show resolved Hide resolved
docs/usage/configuration.md Outdated Show resolved Hide resolved
docs/usage/configuration.md Outdated Show resolved Hide resolved
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.

@AinaMerch okay great progress. I've left some more comments, plus a larger note to focus on formalizing test cases for these changes.

Additionally, please make sure to update the Changelog with a new entry under "Enhancements" to describe this change.

docs/usage/configuration.md Outdated Show resolved Hide resolved
python_ta/__init__.py Outdated Show resolved Hide resolved
python_ta/__init__.py Outdated Show resolved Hide resolved
python_ta/allow_pylint_comments_false_testcase.py Outdated Show resolved Hide resolved
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.

@AinaMerch great, I left a few more comments but I think this is very close!

def _verify_pre_check(filepath: AnyStr, allow_pylint_comments: bool) -> bool:
"""Check student code for certain issues.
The additional allow_pylint_comments parameter indicates whether we want the user to be able to add comments
beginning with pylint which can be used to locally disable checks."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Put the ending """ on a new line (this is the style we use when there's a multi-line docstring)

python_ta/allow_pylint_comments_false_testcase.py Outdated Show resolved Hide resolved

def test_allow_pylint_comments() -> None:
"""Test that checks whether the allow-pylint-comments configuration option works as expected when it is
set to True"""
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, make sure the ending """ is on a new line. Same with the second test case below.

@david-yz-liu david-yz-liu merged commit 863b549 into pyta-uoft:master Nov 18, 2023
6 checks passed
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.

3 participants