Align logging usage to python logging best practices #245

dariopnc · 2021-10-11T13:12:38Z

Ref: https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library

In detail:

completely rewrote logger.py to only consider the WDM_LOG_LEVEL environment variable
modified every call to the logging function to adapt to the new structure
removed unused log_level parameter for Driver object init methods

Downsides:

Lost the abilty to set a fixed formatter for our logs
Lost the difference in logging levels between microsoft drivers and the others
Lost backward compatibilty for direct call to Driver object and its children

These downsides can be addressed by the module user, which is now able to properly control logs

Fixes #209
Closes #210
Fixes #277
Fixes #287

In detail: - completely rewrote logger.py to only consider the WDM_LOG_LEVEL environment variable - modified every call to the logging function to adapt to the new structure - didn't change init methods to be backwards-compatible (log_level parameter is ignored) and added comments explaining it Downsides: - Lost the abilty to set a fixed formatter for our logs - Lost the difference in logging levels between microsoft drivers and the others But these downsides can be addressed by the module user, which is now able to properly control logs

pep8speaks · 2021-10-11T13:12:43Z

Hello @dariopnc! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file webdriver_manager/chrome.py:

Line 31:80: E501 line too long (98 > 79 characters)

In the file webdriver_manager/driver.py:

Line 66:80: E501 line too long (106 > 79 characters)
Line 111:80: E501 line too long (93 > 79 characters)
Line 123:80: E501 line too long (85 > 79 characters)
Line 201:80: E501 line too long (80 > 79 characters)
Line 320:56: E231 missing whitespace after ','
Line 320:67: E231 missing whitespace after ','
Line 320:80: E501 line too long (88 > 79 characters)

In the file webdriver_manager/driver_cache.py:

Line 81:80: E501 line too long (104 > 79 characters)

In the file webdriver_manager/firefox.py:

Line 19:80: E501 line too long (91 > 79 characters)

In the file webdriver_manager/logger.py:

Line 6:1: E302 expected 2 blank lines, found 1
Line 12:1: W293 blank line contains whitespace
Line 13:1: E305 expected 2 blank lines after class or function definition, found 1

In the file webdriver_manager/manager.py:

Line 9:80: E501 line too long (82 > 79 characters)

In the file webdriver_manager/utils.py:

Line 218:69: E231 missing whitespace after ','
Line 218:80: E501 line too long (95 > 79 characters)
Line 218:82: E231 missing whitespace after ','
Line 220:47: E231 missing whitespace after ','
Line 220:60: E231 missing whitespace after ','

Comment last updated at 2022-03-31 07:11:36 UTC

aleksandr-kotlyar · 2021-10-15T07:05:25Z

@dariopnc some tests have been failed cause of "first line" argument.
https://github.com/SergeyPirogov/webdriver_manager/pull/245/checks?check_run_id=3859523313

fail example screenshot

… parameter in logging calls. With the current master version you could have specified the first_line paramter during every logging call, but that would have been ignored for every call other than the one printing the actual first lines. With this version you don't need to specify that parameter at every logging call other than that one.

codecov-commenter · 2021-10-18T15:54:27Z

Codecov Report

Merging #245 (8b74525) into master (259cfdb) will decrease coverage by 0.34%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
- Coverage   93.09%   92.75%   -0.35%     
==========================================
  Files          11       11              
  Lines         478      469       -9     
  Branches       68       66       -2     
==========================================
- Hits          445      435      -10     
- Misses         17       18       +1     
  Partials       16       16

Impacted Files	Coverage Δ
webdriver_manager/chrome.py	`100.00% <100.00%> (ø)`
webdriver_manager/driver.py	`97.50% <100.00%> (ø)`
webdriver_manager/driver_cache.py	`93.50% <100.00%> (ø)`
webdriver_manager/firefox.py	`100.00% <100.00%> (ø)`
webdriver_manager/logger.py	`77.77% <100.00%> (-16.96%)`	⬇️
webdriver_manager/manager.py	`100.00% <100.00%> (ø)`
webdriver_manager/microsoft.py	`100.00% <100.00%> (ø)`
webdriver_manager/opera.py	`88.88% <100.00%> (-0.59%)`	⬇️
webdriver_manager/utils.py	`91.66% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 259cfdb...8b74525. Read the comment docs.

aleksandr-kotlyar · 2021-10-18T16:06:24Z

If log_level not used anymore - we can delete it and make a release note that in new version it has been deleted.

…og_level parameter in drivers calls

dariopnc · 2021-10-18T16:24:03Z

Removed references to log_level as suggested

aleksandr-kotlyar · 2021-10-18T20:02:20Z

@dariopnc i just discovered that all wdm logs have been disappeared. Shouldn't they be enabled by default?

dariopnc · 2021-10-19T09:23:17Z

@aleksandr-kotlyar Thing is logging is enabled, but it's up to the application using this module to configure a logging handler to receive log messages and handle them (add them to a file, print them in the console, send them through SOAP calls, etc)
Since automated tests seem to depend on parsing logging messages, we might need to modify the test definitions.

dariopnc · 2021-11-15T11:36:19Z

@aleksandr-kotlyar To merge this change should I update test cases? Should I wait for someone else to do it?

Changes to log calls to adequate to the use of a central logger

Use the new object and logging standard functions

aleksandr-kotlyar · 2022-02-07T10:52:17Z

@dariopnc thanks for keeping your PR up to date!

I think about - how to preprare webdriver-manager users for these changes.

Whats breaking changes and how to make it work again? - this info can be in PR description and in CHANGELOG.md
How to fully disable logs now - this info needs to be in README.md

It will be cool if you will help to describe this info in this PR before merge and release it.

dariopnc · 2022-02-07T16:27:37Z

@aleksandr-kotlyar Updated doc as suggested but now I sincerely don't know why test_utils is failing
Any suggestion regarding both documentation and test is well received :)

dariopnc · 2022-02-07T16:33:38Z

Ok, I suppose 9f405f2 from current master is to blame for the issue... actually even current HEAD fails some tests

aleksandr-kotlyar · 2022-02-08T11:31:35Z

@aleksandr-kotlyar Updated doc as suggested but now I sincerely don't know why test_utils is failing Any suggestion regarding both documentation and test is well received :)

Thank you for your work.

I fixed test utils in new PR #298. Pull the changes please to resolve conflicts.

dariopnc · 2022-02-09T15:12:10Z

PR updated to current master

dariopnc · 2022-02-16T14:56:08Z

This PR should also solve (or help solve) #277 and #287

dariopnc · 2022-03-14T09:34:59Z

Hi @aleksandr-kotlyar, I've seen #343 significantly modifies the logging structure. I sincerely do not understand why this PR hasn't been considered, instead on adding a new custom behavior. Can you please explain?
The desired behavior (silent all logs) could have been achieved with this PR, by just setting WDM_LOG_LEVEL='0' as documented in the README.md file

Dario Panico added 2 commits October 11, 2021 14:56

Edited comments to satisfy PEP8

4187da8

As per @aleksandr-kotlyar comment I removed every reference for the l…

7bae7ac

…og_level parameter in drivers calls

dariopnc and others added 6 commits November 17, 2021 17:10

Merge branch 'master' into master

4c108ba

Merge branch 'master' into master

4edeab4

Fix remaining logger calls

d07a7fc

Changes to log calls to adequate to the use of a central logger

Fixed log call in get_latest_release_version

bd5bd59

Use the new object and logging standard functions

Uniform log call notations to avoid incompatibilities

64c4714

Merge branch 'master' into master

34f1921

dariopnc mentioned this pull request Feb 7, 2022

would like to pass logger name equals to none so it will use root logger #210

Closed

dariopnc added 2 commits February 7, 2022 16:57

Updated PR documentation in both README and CHANGELOG file

f22f67a

Merge branch 'master' of https://github.com/dariopnc/webdriver_manager

901b782

Merge branch 'master' into master

a2c401c

Merge branch 'master' into master

d105d5e

aleksandr-kotlyar mentioned this pull request Feb 16, 2022

can't really disable logs #209

Closed

dariopnc changed the title ~~Aligned logging usage to python logging best practices~~ Align logging usage to python logging best practices Mar 10, 2022

Merge branch 'master' into master

79db515

dariopnc and others added 3 commits March 16, 2022 11:00

Changed the test to use the correct logging facility.

8ad84d6

Edited Changelog as well

444b963

Added to .gitignore paths and files generated during testing

d199a83

dariopnc force-pushed the master branch from 80cb682 to d199a83 Compare March 16, 2022 18:57

dariopnc added 3 commits March 17, 2022 11:31

Merge branch 'master' into master

ed87ee8

Fixed error in conflict resolution

06bc573

Merge branch 'master' into master

8b74525

SergeyPirogov closed this May 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align logging usage to python logging best practices #245

Align logging usage to python logging best practices #245

dariopnc commented Oct 11, 2021 •

edited

Loading

pep8speaks commented Oct 11, 2021 •

edited

Loading

aleksandr-kotlyar commented Oct 15, 2021 •

edited

Loading

codecov-commenter commented Oct 18, 2021 •

edited

Loading

aleksandr-kotlyar commented Oct 18, 2021

dariopnc commented Oct 18, 2021

aleksandr-kotlyar commented Oct 18, 2021

dariopnc commented Oct 19, 2021 •

edited

Loading

dariopnc commented Nov 15, 2021

aleksandr-kotlyar commented Feb 7, 2022 •

edited

Loading

dariopnc commented Feb 7, 2022

dariopnc commented Feb 7, 2022

aleksandr-kotlyar commented Feb 8, 2022

dariopnc commented Feb 9, 2022

dariopnc commented Feb 16, 2022

dariopnc commented Mar 14, 2022 •

edited

Loading

Align logging usage to python logging best practices #245

Align logging usage to python logging best practices #245

Conversation

dariopnc commented Oct 11, 2021 • edited Loading

pep8speaks commented Oct 11, 2021 • edited Loading

Comment last updated at 2022-03-31 07:11:36 UTC

aleksandr-kotlyar commented Oct 15, 2021 • edited Loading

codecov-commenter commented Oct 18, 2021 • edited Loading

Codecov Report

aleksandr-kotlyar commented Oct 18, 2021

dariopnc commented Oct 18, 2021

aleksandr-kotlyar commented Oct 18, 2021

dariopnc commented Oct 19, 2021 • edited Loading

dariopnc commented Nov 15, 2021

aleksandr-kotlyar commented Feb 7, 2022 • edited Loading

dariopnc commented Feb 7, 2022

dariopnc commented Feb 7, 2022

aleksandr-kotlyar commented Feb 8, 2022

dariopnc commented Feb 9, 2022

dariopnc commented Feb 16, 2022

dariopnc commented Mar 14, 2022 • edited Loading

dariopnc commented Oct 11, 2021 •

edited

Loading

pep8speaks commented Oct 11, 2021 •

edited

Loading

aleksandr-kotlyar commented Oct 15, 2021 •

edited

Loading

codecov-commenter commented Oct 18, 2021 •

edited

Loading

dariopnc commented Oct 19, 2021 •

edited

Loading

aleksandr-kotlyar commented Feb 7, 2022 •

edited

Loading

dariopnc commented Mar 14, 2022 •

edited

Loading