-
Notifications
You must be signed in to change notification settings - Fork 58
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
Replacing "print" with "logging" Module #974
Changes from 33 commits
08c3d3c
fed6c77
385cde6
f3761bf
8f33d56
b83198b
91b64d5
c0d1375
007c3fa
3c1dc0e
0ee4c29
13e63f9
81e166a
9aff6a9
86ecdca
bc13e8c
ab7a72c
f18f687
12edcc6
e6b09e3
8c00984
e469668
972ec39
c95b494
36c4c38
ed5ff7d
a38c471
4949be1
8f5cce3
76f1b4e
7cd3045
e4128be
a59aaf5
1fb8b82
9a0bc41
c527359
8701320
3e7ce89
b13d09c
e5dd549
4523746
91125e0
2a3e58c
01bfeb5
efd7245
6e7ea88
88c4cc8
4a580c4
e9c25f1
d8ff8a9
6720186
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
Within the config submodule, this .py file encompasses functions responsible | ||
for managing all configuration-related tasks. | ||
""" | ||
|
||
import logging | ||
import os | ||
import sys | ||
from pathlib import Path | ||
|
@@ -48,7 +48,7 @@ def override_config(linter: PyLinter, config_location: AnyStr) -> None: | |
try: | ||
_, config_args = config_file_parser.parse_config_file(file_path=config_location) | ||
except OSError as ex: | ||
print(ex, file=sys.stderr) | ||
logging.error(ex) | ||
sys.exit(32) | ||
|
||
# Override the config options by parsing the provided file. | ||
|
@@ -76,7 +76,7 @@ def load_messages_config(path: str, default_path: str, use_pyta_error_messages: | |
try: | ||
merge_from = toml.load(path) | ||
except FileNotFoundError: | ||
print(f"[WARNING] Could not find messages config file at {str(Path(path).resolve())}.") | ||
logging.warning(f"Could not find messages config file at {str(Path(path).resolve())}. ") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Delete the space at the end of the string |
||
merge_from = {} | ||
|
||
if not use_pyta_error_messages: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
# pylint: disable=unbalanced-tuple-unpacking |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
"�" | ||
"�" | ||
"foo" | ||
"foo" | ||
"�" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,16 @@ | |
Test suite for checking whether configuration worked correctly with user-inputted configurations. | ||
""" | ||
import json | ||
import logging | ||
import os | ||
from unittest.mock import mock_open, patch | ||
|
||
import pylint.lint | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You do not need both this and the other |
||
import pytest | ||
from pylint import lint | ||
|
||
import python_ta | ||
from python_ta.config import load_messages_config, override_config | ||
|
||
TEST_CONFIG = { | ||
"pyta-number-of-messages": 10, | ||
|
@@ -229,6 +233,26 @@ def test_config_parse_error_has_no_snippet() -> None: | |
assert snippet == "" | ||
|
||
|
||
def test_override_config_logging(caplog) -> None: | ||
"""Testing that the OSError in override_config is logged correctly""" | ||
path = "C:\\foo\\tests\\file_fixtures\\test_f0011.pylintrc" | ||
linter = lint.PyLinter() | ||
|
||
with pytest.raises(SystemExit): | ||
override_config(linter, path) | ||
assert caplog.records[0].levelname == "ERROR" | ||
assert f"The config file {path} doesn't exist!" in caplog.text | ||
|
||
|
||
@patch("python_ta.config.toml.load", side_effect=FileNotFoundError) | ||
def test_load_messages_config_logging(_, caplog): | ||
try: | ||
load_messages_config("non_existent_file.toml", "default_file.toml", True) | ||
except FileNotFoundError: | ||
assert "Could not find messages config file at" in caplog.text | ||
assert "WARNING" in [record.levelname for record in caplog.records] | ||
|
||
|
||
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
from python_ta import contracts | ||
|
||
contracts.DEBUG_CONTRACTS = True | ||
from python_ta.contracts import check_contracts | ||
|
||
|
||
def test_contracts_debug(caplog) -> None: | ||
"""Test to see if _debug method is logging messages correctly""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
@check_contracts | ||
def divide(x: int, y: int) -> int: | ||
"""Return x // y. | ||
|
||
Preconditions: | ||
- invalid precondition | ||
""" | ||
return x // y | ||
|
||
divide(6, 2) | ||
|
||
for record in caplog.records: | ||
assert record.levelname == "DEBUG" | ||
assert ( | ||
"Warning: precondition invalid precondition could not be parsed as a valid Python expression" | ||
in caplog.text | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
"""Tests for top level __init__.py logging functionality in pyta""" | ||
import os | ||
import tokenize | ||
from unittest.mock import patch | ||
|
||
import python_ta | ||
from python_ta import _get_valid_files_to_check, _verify_pre_check | ||
|
||
|
||
def test_check_log(caplog) -> None: | ||
"""Testing logging in _check function when no exception is thrown""" | ||
expected_messages = [ | ||
"was checked using the configuration file:", | ||
"was checked using the messages-config file:", | ||
] | ||
|
||
python_ta._check() | ||
for i in range(2): | ||
assert caplog.records[i].levelname == "INFO" | ||
assert expected_messages[i] in caplog.records[i].msg | ||
|
||
|
||
@patch("python_ta._get_valid_files_to_check", side_effect=Exception("Testing")) | ||
def test_check_exception_log(_, caplog) -> None: | ||
"""Testing logging in _check function when exception is thrown""" | ||
try: | ||
python_ta._check() | ||
except Exception: | ||
expected_logs = [ | ||
"Unexpected error encountered! Please report this to your instructor (and attach the code that caused the error).", | ||
'Error message: "Testing"', | ||
] | ||
|
||
for i in range(2): | ||
assert caplog.records[i].levelname == "ERROR" | ||
assert expected_logs[i] in caplog.records[i].msg | ||
|
||
|
||
def test_pre_check_log_pylint_comment(caplog) -> None: | ||
"""Testing logging in _verify_pre_check function when checking for pyling comment""" | ||
path = os.path.join(os.path.dirname(__file__), "fixtures", "pylint_comment.py") | ||
_verify_pre_check(path, False) | ||
assert f'String "pylint:" found in comment. No check run on file `{path}' in caplog.text | ||
assert "ERROR" == caplog.records[0].levelname | ||
|
||
|
||
@patch("python_ta.tokenize.open", side_effect=IndentationError) | ||
def test_pre_check_log_indentation_error(_, caplog) -> None: | ||
"""Testing logging in _verify_pre_check function IndentationError catch block""" | ||
# Don't need a valid file path since patching error into open function | ||
_verify_pre_check("", False) | ||
assert "python_ta could not check your code due to an indentation error at line" in caplog.text | ||
assert "ERROR" == caplog.records[0].levelname | ||
|
||
|
||
@patch("python_ta.tokenize.open", side_effect=tokenize.TokenError) | ||
def test_pre_check_log_token_error(_, caplog) -> None: | ||
"""Testing logging in _verify_pre_check function TokenError catch block""" | ||
# Don't need a valid file path since patching error into open function | ||
_verify_pre_check("", False) | ||
assert "python_ta could not check your code due to a syntax error in your file." in caplog.text | ||
assert "ERROR" == caplog.records[0].levelname | ||
|
||
|
||
@patch("python_ta.tokenize.open", side_effect=UnicodeDecodeError("", b"", 0, 0, "")) | ||
def test_pre_check_log_pylint_unicode_error(_, caplog) -> None: | ||
"""Testing logging in _verify_pre_check function UnicodeDecodeError catch block""" | ||
expected_logs = [ | ||
"python_ta could not check your code due to an invalid character. Please check the following lines in your file and all characters that are marked with a �.", | ||
' Line 1: "�"\n', | ||
' Line 2: "�"\n', | ||
' Line 5: "�"', | ||
] | ||
|
||
path = os.path.join(os.path.dirname(__file__), "fixtures", "unicode_decode_error.py") | ||
_verify_pre_check(path, False) | ||
|
||
for i in range(len(expected_logs)): | ||
assert expected_logs[i] == caplog.records[i].msg | ||
assert "ERROR" == caplog.records[i].levelname | ||
|
||
|
||
def test_get_valid_files_to_check(caplog) -> None: | ||
"""Testing logging in _get_valid_files_to_check function""" | ||
expected_logs = [ | ||
"No checks run. Input to check, `{'examples/nodes/assign'}`, has invalid type, must be a list of strings.", | ||
"No check run on file `0`, with invalid type. Must be type: str.", | ||
"Could not find the file called, `foo`", | ||
] | ||
|
||
# Iterating through generators to produce errors | ||
[_ for _ in _get_valid_files_to_check(module_name={"examples/nodes/assign"})] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For a generator, you can execute until the first There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Neat. I didn't know about that. I also just looked it up, I could also just pass the generator function into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @merrickliu888 sorry I missed this! Using |
||
[_ for _ in _get_valid_files_to_check(module_name=[0])] | ||
[_ for _ in _get_valid_files_to_check(module_name="foo")] | ||
|
||
for i in range(len(expected_logs)): | ||
assert caplog.records[i].levelname == "ERROR" | ||
assert expected_logs[i] in caplog.records[i].msg | ||
|
||
|
||
def test_doc_log(caplog) -> None: | ||
"""Testing logging in doc function""" | ||
python_ta.doc("E0602") | ||
assert caplog.records[0].levelname == "INFO" | ||
assert ( | ||
"Opening http://www.cs.toronto.edu/~david/pyta/checkers/index.html#e0602 in a browser." | ||
in caplog.text | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
"""Test for __init__.py system version check""" | ||
import importlib | ||
import sys | ||
|
||
import python_ta | ||
|
||
|
||
def test_sys_version_log(caplog, monkeypatch) -> None: | ||
"""Testing if message logged when system version is too low is correct""" | ||
monkeypatch.setattr(sys, "version_info", (2, 6, 0)) | ||
importlib.reload(python_ta) # Necessary due to python's import not actually reimporting package | ||
|
||
assert caplog.records[0].levelname == "WARNING" | ||
assert "You need Python 3.7 or later to run PythonTA." in caplog.text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this, I realized that we really shouldn't have this top-level code at all.
_check
method.sys.version_info < (3, 7, 0)
part there as well.sys.version_info
test a lot simpler...! 😅