Replacing "print" with "logging" Module #974

merrickliu888 · 2023-10-20T17:44:35Z

Motivation and Context

"print" only prints out text, while "logging" is able to store meta data with their messages, such as the time and line of code where a "logging" message was outputted. "logging" provides more functionality and extensibility for future changes regarding user interaction with PyTA, and debugging runtime issues.

Your Changes

Replaced all "print" calls with the appropriate "logging" method in __init__.py, configs/, and contracts/.
In addition, fixed a small bug regarding the reporting of the UnicodeDecodeError in the _verify_pre_check function in the top level __init__.py of python_ta. The issue was that when the lines that produced the UnicodeDecodeError were logged, the line number was one off as enumerate is 0-indexed, rather than starting at 1. I fixed it by adding 1 to i.

Moved the sys.version_info if statement from being a top level statement in the __init__.py of python_ta into the _check function.

Type of change (select all that apply):

Refactoring (internal change to codebase, without changing functionality)
Test update (change that modifies or updates tests only)

Testing

I tested these changes by manually triggering the "logging" and creating new unit tests for each "logging" call to verify that the message was logged correctly (message content and logging level ie. "ERROR", "INFO", etc.)

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.

for more information, see https://pre-commit.ci

…into logging

for more information, see https://pre-commit.ci

david-yz-liu

Good work @merrickliu888, here is some feedback on the failing tests.

The first set of tests (in test_messages_config.py) are failing because they are checking whether specific error messages are being printed to stdout, which I suppose isn't happening now with the logging module. The easiest fix to this is to change the subprocess.run call to a python_ta.check_all call, with the appropriate arguments (the test file and the configuration file). Then, use the same pytest fixture to capture the logging as you've done with the other tests.
You're likely correct about the monkey-patching for the sys.version_info interfering with other test cases. Doing a test teardown that again reloads the python_ta module (with the monkey patching disabled) is a great idea.
As for the failing GitHub CI tests, this is a new problem with z3 (Python bindings: Missing importlib_resources dependency Z3Prover/z3#7041). You can fix this for now by modifying the pyproject.toml file's z3-solver dependency to be z3-solver==4.12.1.0. (This is temporary---I'll make a PR to fix this separately, but I don't want it to hold up your PR.)

merrickliu888 · 2023-12-06T22:06:12Z

@david-yz-liu I did some investigation for the tests that were failing in test_messages_config.py and it seems the reason its failing is because of the path to the files they need to access. At least locally. I need to see if this error happens in GitHub.

…into logging

for more information, see https://pre-commit.ci

coveralls · 2023-12-06T22:30:43Z

Pull Request Test Coverage Report for Build 7161535366

29 of 29 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 94.837%

Totals
Change from base Build 7160350456:	0.2%
Covered Lines:	3453
Relevant Lines:	3641

💛 - Coveralls

…into logging

david-yz-liu

@merrickliu888 great work! I left some fairly minor comments here

david-yz-liu · 2023-12-07T21:12:25Z

python_ta/__init__.py

@@ -53,11 +54,14 @@
 from .reporters import REPORTERS
 from .upload import upload_to_server

+# Configuring logger


Looking at this, I realized that we really shouldn't have this top-level code at all.

Move this to the top of the _check method.

Move the sys.version_info < (3, 7, 0) part there as well.

I realize this actually makes your sys.version_info test a lot simpler...! 😅

david-yz-liu · 2023-12-07T21:14:20Z

python_ta/config/__init__.py

@@ -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())}. ")


Delete the space at the end of the string

david-yz-liu · 2023-12-07T21:15:54Z

tests/test_config/test_config.py

 import os
 from unittest.mock import mock_open, patch

+import pylint.lint


You do not need both this and the other pylint import statement you added. The typical idiom is import pylint.lint as lint

david-yz-liu · 2023-12-07T21:16:50Z

tests/test_contracts/test_contracts_debug.py

+
+
+def test_contracts_debug(caplog) -> None:
+    """Test to see if _debug method is logging messages correctly"""


_debug is a function, but not a method

david-yz-liu · 2023-12-07T21:20:41Z

tests/test_init_logging.py

+    ]
+
+    # Iterating through generators to produce errors
+    [_ for _ in _get_valid_files_to_check(module_name={"examples/nodes/assign"})]


For a generator, you can execute until the first yield by calling next, e.g. next(_get_valid_files_to_check(...))

Neat. I didn't know about that. I also just looked it up, I could also just pass the generator function into tuple(). In this case, would it be better to use a while loop and next, or keep the code minimal and just use the tuple() constructor and "one-line" it?

@merrickliu888 sorry I missed this! Using tuple was totally fine. :)

- Revert python_ta.doc function to print message to stdout - Move logging config into functions (rather than at top-level) - Update tests and fix Windows encoding issue in test cases

merrickliu888 added 15 commits October 9, 2023 21:07

init logging

08c3d3c

formatting

fed6c77

contracts logger

385cde6

config logger

f3761bf

contracts logging test

8f33d56

cleaning up comments

b83198b

adding return val

91b64d5

removing original prefixes for msgs

c0d1375

Adding logging

007c3fa

Adding logging config back and taking out last print statement

3c1dc0e

Top level init tests

0ee4c29

cleaning up TODOs

13e63f9

init version test

81e166a

tests

9aff6a9

Merge

86ecdca

merrickliu888 changed the title ~~Replacing "print" with "logging" Module~~ Replacing "print" with "logging" Module (Draft) Oct 20, 2023

merrickliu888 added 2 commits October 29, 2023 18:16

test_load_messages test fixed using patch

bc13e8c

test_load_messages test fixed using patch

ab7a72c

merrickliu888 marked this pull request as draft November 15, 2023 19:20

merrickliu888 and others added 8 commits November 28, 2023 11:19

Reformatting

f18f687

[pre-commit.ci] auto fixes from pre-commit.com hooks

12edcc6

for more information, see https://pre-commit.ci

test config done

e6b09e3

test valid files done

8c00984

test _check logging finished

e469668

adding pylint_comment example file and finish testing

972ec39

merge

c95b494

[pre-commit.ci] auto fixes from pre-commit.com hooks

36c4c38

for more information, see https://pre-commit.ci

merrickliu888 marked this pull request as ready for review November 28, 2023 17:52

merrickliu888 added 2 commits November 30, 2023 11:57

merge

ed5ff7d

pathing fix

a38c471

merrickliu888 and others added 4 commits December 5, 2023 00:40

init logging fix

76f1b4e

init logging unicode bug fix

7cd3045

Merge branch 'logging' of https://github.com/merrickliu888/pyta-fork …

e4128be

…into logging

[pre-commit.ci] auto fixes from pre-commit.com hooks

a59aaf5

for more information, see https://pre-commit.ci

merrickliu888 requested a review from david-yz-liu December 5, 2023 06:09

david-yz-liu reviewed Dec 5, 2023

View reviewed changes

fixing paths

1fb8b82

merrickliu888 and others added 5 commits December 6, 2023 17:07

fixing z3-solver version issue

9a0bc41

adding back path

c527359

Merge branch 'logging' of https://github.com/merrickliu888/pyta-fork …

8701320

…into logging

fixing tests after commit hooks

3e7ce89

[pre-commit.ci] auto fixes from pre-commit.com hooks

b13d09c

for more information, see https://pre-commit.ci

merrickliu888 added 3 commits December 7, 2023 11:42

adding monkepatch.undo()

e5dd549

Merge branch 'logging' of https://github.com/merrickliu888/pyta-fork …

4523746

…into logging

merge

91125e0

david-yz-liu reviewed Dec 7, 2023

View reviewed changes

merrickliu888 added 6 commits December 7, 2023 17:50

fixing space

2a3e58c

moving version logging

01bfeb5

fixing comment

efd7245

fixing init testing

6e7ea88

spacing

88c4cc8

removing import

4a580c4

david-yz-liu changed the title ~~Replacing "print" with "logging" Module (Draft)~~ Replacing "print" with "logging" Module Dec 10, 2023

david-yz-liu added 3 commits December 10, 2023 16:48

Merge remote-tracking branch 'origin/master' into logging

e9c25f1

Move logging config into _check function

d8ff8a9

Some updates to logging branch.

6720186

- Revert python_ta.doc function to print message to stdout - Move logging config into functions (rather than at top-level) - Update tests and fix Windows encoding issue in test cases

david-yz-liu approved these changes Dec 11, 2023

View reviewed changes

david-yz-liu merged commit 2ce5460 into pyta-uoft:master Dec 11, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing "print" with "logging" Module #974

Replacing "print" with "logging" Module #974

merrickliu888 commented Oct 20, 2023 •

edited

Loading

david-yz-liu left a comment

merrickliu888 commented Dec 6, 2023 •

edited

Loading

coveralls commented Dec 6, 2023 •

edited

Loading

david-yz-liu left a comment

david-yz-liu Dec 7, 2023

david-yz-liu Dec 7, 2023

david-yz-liu Dec 7, 2023

david-yz-liu Dec 7, 2023

david-yz-liu Dec 7, 2023

merrickliu888 Dec 7, 2023

david-yz-liu Dec 11, 2023



		def test_contracts_debug(caplog) -> None:
		"""Test to see if _debug method is logging messages correctly"""

Replacing "print" with "logging" Module #974

Replacing "print" with "logging" Module #974

Conversation

merrickliu888 commented Oct 20, 2023 • edited Loading

Motivation and Context

Your Changes

Testing

Checklist

david-yz-liu left a comment

Choose a reason for hiding this comment

merrickliu888 commented Dec 6, 2023 • edited Loading

coveralls commented Dec 6, 2023 • edited Loading

Pull Request Test Coverage Report for Build 7161535366

💛 - Coveralls

david-yz-liu left a comment

Choose a reason for hiding this comment

david-yz-liu Dec 7, 2023

Choose a reason for hiding this comment

david-yz-liu Dec 7, 2023

Choose a reason for hiding this comment

david-yz-liu Dec 7, 2023

Choose a reason for hiding this comment

david-yz-liu Dec 7, 2023

Choose a reason for hiding this comment

david-yz-liu Dec 7, 2023

Choose a reason for hiding this comment

merrickliu888 Dec 7, 2023

Choose a reason for hiding this comment

david-yz-liu Dec 11, 2023

Choose a reason for hiding this comment

merrickliu888 commented Oct 20, 2023 •

edited

Loading

merrickliu888 commented Dec 6, 2023 •

edited

Loading

coveralls commented Dec 6, 2023 •

edited

Loading