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

improve the log configuration with using RotatingFileHandler. #4844

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PyCatFish
Copy link

@PyCatFish PyCatFish commented Oct 15, 2020

Add new options about log so that users can set the size and backup count of log file.

Resolves #3628

@codecov
Copy link

codecov bot commented Oct 15, 2020

Codecov Report

Merging #4844 into master will decrease coverage by 0.03%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #4844      +/-   ##
==========================================
- Coverage   87.85%   87.82%   -0.04%     
==========================================
  Files         160      160              
  Lines        9735     9740       +5     
  Branches     1437     1438       +1     
==========================================
+ Hits         8553     8554       +1     
- Misses        925      928       +3     
- Partials      257      258       +1     
Impacted Files Coverage Δ
scrapy/utils/log.py 85.71% <33.33%> (-3.54%) ⬇️

@PyCatFish
Copy link
Author

@Gallaecio hi, there is a check , codecov/patch, not successful, is it must be successful then the request can be merged?

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Nice work!

The final step would be to cover the changes in tests (which should make the codecov/patch check pass). In this case it may not be trivial, though. Please, let me know if you get stuck, maybe I can look into it myself when I have some time.

Comment on lines 4 to 6
from logging.config import dictConfig

from logging.handlers import RotatingFileHandler
from twisted.python import log as twisted_log
Copy link
Member

Choose a reason for hiding this comment

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

💄 Please, keep the empty line separating imports from the standard library from imports from third-party libraries.

@@ -122,7 +122,14 @@ def _get_handler(settings):
filename = settings.get('LOG_FILE')
if filename:
encoding = settings.get('LOG_ENCODING')
handler = logging.FileHandler(filename, encoding=encoding)
if settings.get("LOG_ROTATING") is True:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if settings.get("LOG_ROTATING") is True:
if settings.getbool("LOG_ROTATING") is True:

This makes it possible to set the setting from the command line, where every value is a string.

@@ -154,7 +157,11 @@ These settings can be used to configure the logging:
The first couple of settings define a destination for log messages. If
:setting:`LOG_FILE` is set, messages sent through the root logger will be
redirected to a file named :setting:`LOG_FILE` with encoding
:setting:`LOG_ENCODING`. If unset and :setting:`LOG_ENABLED` is ``True``, log
:setting:`LOG_ENCODING`. If :setting:`LOG_FILE` is set and
:setting:`LOG_ROTATING` is ```True```, the file named
Copy link
Member

Choose a reason for hiding this comment

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

💄

Suggested change
:setting:`LOG_ROTATING` is ```True```, the file named
:setting:`LOG_ROTATING` is ``True``, the file named

@yushao2
Copy link

yushao2 commented Jan 30, 2023

@Gallaecio I made a new PR to pick this up since this is stale

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.

Crawler logs are cut by day
3 participants