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

Merge user's input config with pyta's default options #914

Merged
merged 5 commits into from
Jun 20, 2023

Conversation

sushimon
Copy link
Contributor

@sushimon sushimon commented Jun 14, 2023

Motivation and Context

The current configuration behaviour will completely overwrite all of pyta's default options if the user provides their own config options. As a result, the user must provide a lengthy config file containing all of pyta's default options, along with any overridden ones. These changes allows the user to provide a very small config file containing only the options they want overridden.

Your Changes

Description:

  • Modified pyta's configuration behaviour to always load the default .pylintrc file first, before applying any user-specified configuration options, if provided.
  • Added test cases to verify that both methods of applying configurations correctly update the linter's config options.
  • Updated the documentation of configuration to provide more information about the new configuration behaviour changes.

Type of change (select all that apply):

  • Refactoring (internal change to codebase, without changing functionality)

Testing

  • Ran the test suite and verified the test cases passed.
  • Ran the newly added test cases and verified they passed.
  • Ran python_ta.check_all calls with an inputted config dict and a new .pylintrc file and verified the specified configuration options were correctly applied to the linter.

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.
  • I have updated the CHANGELOG.md file.

@coveralls
Copy link
Collaborator

coveralls commented Jun 14, 2023

Pull Request Test Coverage Report for Build 5315534521

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

Totals Coverage Status
Change from base Build 5236184120: 0.07%
Covered Lines: 1747
Relevant Lines: 1952

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

@sushimon good work! I left one comment about the logic. But also I realized that to preserve backwards compatibility it would be good to provide an option to not use the PythonTA default config. That could be an optional argument to _check (and, transitively, check_all/check_errors) like load_default_config (with default value True).

python_ta/__init__.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.

@sushimon good job! I left another review but I think you're pretty close.

In addition to the inline comments, please make sure to add some tests for load_default_config=False to the test cases you added.

CHANGELOG.md Show resolved Hide resolved
docs/usage/configuration.md Outdated Show resolved Hide resolved
python_ta/__init__.py Show resolved Hide resolved
python_ta/__init__.py Outdated Show resolved Hide resolved
@david-yz-liu david-yz-liu merged commit 19536af into pyta-uoft:master Jun 20, 2023
5 checks passed
@sushimon sushimon deleted the merge-input-config branch June 20, 2023 18:57
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