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

[MRG+1] docs for scrapy.logformatter #3660

Merged
merged 5 commits into from
Jul 23, 2019

Conversation

anubhavp28
Copy link
Contributor

Fixes #3616 by documenting logFormatter class.

@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #3660 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3660   +/-   ##
=======================================
  Coverage   84.52%   84.52%           
=======================================
  Files         167      167           
  Lines        9410     9410           
  Branches     1397     1397           
=======================================
  Hits         7954     7954           
  Misses       1199     1199           
  Partials      257      257
Impacted Files Coverage Δ
scrapy/logformatter.py 90.47% <ø> (ø) ⬆️

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.

Thank you for your work! Please, have a look at my feedback.

Custom Log Formats
-------------------

Custom log format can be set for different actions by extending ``scrapy.logformatter.LogFormatter`` class.
Copy link
Member

Choose a reason for hiding this comment

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

I think

:class:`~scrapy.logformatter.LogFormatter`

yould be better than

``scrapy.logformatter.LogFormatter``

specially now that you have provided API reference documentation for that class (thanks!).

Listed below is details of what each key represents :

* ``level`` is the log level for that action, you can use those from the python logging library:
:setting:`logging.DEBUG`, :setting:`logging.INFO`, :setting:`logging.WARNING`, :setting:`logging.ERROR`
Copy link
Member

Choose a reason for hiding this comment

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

:setting: is a custom syntax we have in Scrapy for Scrapy settings. Here you should use regular code formatting, for example:

``logging.DEBUG``

Consider, however, linking the first mention of Python’s logging module to its documentation. There users can find more information about these constants.

* ``args`` should be a tuple or dict with the formatting placeholders for `msg`. The final log message is
computed as ``msg % args``.

.. note:: To use custom log formatting class, you must mention it in ``settings.py``, by adding a line
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think this should be a note. I believe notes should be used when the corresponding text does not require them. Here, the content of this note is a mandatory step in setting up a custom log formatter.

Also, as part of your changes you should cover this setting, LOG_FORMATTER, in settings.rst. When you do that, you should be able to link to it using:

:setting:`LOG_FORMATTER`

.. note:: To use custom log formatting class, you must mention it in ``settings.py``, by adding a line
``LOG_FORMATTER = '<path_to_your_class>’``

.. class:: scrapy.logformatter.LogFormatter
Copy link
Member

Choose a reason for hiding this comment

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

In #3612 we aim to keep API reference documentation in the source code and include it in the documentation with autodoc. Please, consider moving this documentation to the LogFormatter class; then you should be able to include it here with something in the lines of:

.. autoclass:: scrapy.logformatter.LogFormatter
   :members:

@anubhavp28
Copy link
Contributor Author

anubhavp28 commented Mar 7, 2019

@Gallaecio I have made changes, can you review this again?

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.

Please, see my second round of feedback. Sorry for being so picky 😅

@@ -193,6 +193,15 @@ to override some of the Scrapy settings regarding logging.
Module `logging.handlers <https://docs.python.org/2/library/logging.handlers.html>`_
Further documentation on available handlers

Custom Log Formats
-------------------
Copy link
Member

Choose a reason for hiding this comment

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

Please, align the hyphens with the section title text:

Suggested change
-------------------
------------------

Custom Log Formats
-------------------

Custom log format can be set for different actions by extending :class:`~scrapy.logformatter.LogFormatter` class
Copy link
Member

Choose a reason for hiding this comment

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

Don’t forget to use articles.

Suggested change
Custom log format can be set for different actions by extending :class:`~scrapy.logformatter.LogFormatter` class
A custom log format can be set for different actions by extending the :class:`~scrapy.logformatter.LogFormatter` class

-------------------

Custom log format can be set for different actions by extending :class:`~scrapy.logformatter.LogFormatter` class
and making :setting:`LOG_FORMATTER` inside ``settings.py`` point to your new class.
Copy link
Member

Choose a reason for hiding this comment

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

Settings can be defined outside settings.py, for example within spiders, so here less is more 😄

Suggested change
and making :setting:`LOG_FORMATTER` inside ``settings.py`` point to your new class.
and making :setting:`LOG_FORMATTER` point to your new class.

LOG_FORMATTER
-------------

Default: ``scrapy.logformatter.LogFormatter``
Copy link
Member

Choose a reason for hiding this comment

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

Since we can now link to it…

Suggested change
Default: ``scrapy.logformatter.LogFormatter``
Default: :class:`scrapy.logformatter.LogFormatter`

docs/topics/logging.rst Show resolved Hide resolved

Default: ``scrapy.logformatter.LogFormatter``

The class to use for formatting log messages for different actions.
Copy link
Member

Choose a reason for hiding this comment

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

Link to the topic you created.

Suggested change
The class to use for formatting log messages for different actions.
The class to use for :ref:`formatting log messages <custom-log-formats>` for different actions.

"""

def crawled(self, request, response, spider):
"""
Copy link
Member

@Gallaecio Gallaecio Mar 8, 2019

Choose a reason for hiding this comment

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

Please, use the same doc string quoting we use all over. That is:

def function1():
    """Triple quotes stay on the same line for the first paragraph."""
    pass

def function2():
    """And things only change when there are two paragraphs.

    Then the last set of triple quotes goes after the last line. But the first stays the same.
    """
    pass

This applies to the two other methods below as well.


* `args` should be a tuple or dict with the formatting placeholders for
`msg`. The final log message is computed as output['msg'] %
output['args'].
"""
Copy link
Member

Choose a reason for hiding this comment

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

Here I am missing a simple example. Examples are a really powerful tool; people very familiar with Scrapy may see the example and the setting name, and already know what to do, without actually reading the remaining text.

You could feature something in the lines of the example from Stackoverflow, as we know for sure that some users will find it useful.

"""

def crawled(self, request, response, spider):
"""
``crawled`` is called to log message when the crawler finds a webpage.
Copy link
Member

Choose a reason for hiding this comment

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

No need to repeat the method name in the doc string.

Suggested change
``crawled`` is called to log message when the crawler finds a webpage.
Logs a message when the crawler finds a webpage.

The same applies to the two methods below as well.

@anubhavp28
Copy link
Contributor Author

anubhavp28 commented Mar 11, 2019

Thank you @Gallaecio for all the help. Can you suggest me some issues/missing features I could fix/implement? I unable to find anything under issues. I want to contribute more to Scrapy. I am looking to apply for GSoC under Scrapy.

@Gallaecio Gallaecio changed the title docs for scrapy.logformatter [MRG+1] docs for scrapy.logformatter Mar 15, 2019
@Gallaecio
Copy link
Member

@anubhavp28 If you cannot find any open issue that interests you, you could always send pull requests to increment automated test coverage, or look into the most voted StackOverflow issues, some of which could warrant documentation changes or even feature implementations or bug fixes.

@kmike
Copy link
Member

kmike commented Jul 23, 2019

Thanks @anubhavp28, and thanks @Gallaecio for the review!

@kmike kmike merged commit c679aef into scrapy:master Jul 23, 2019
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.

Document LogFormatter
3 participants