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

issue #6323: add SpiderLoggerAdapter, change Spider.logger to return SpiderLoggerAdapter #6324

Merged
merged 7 commits into from May 13, 2024

Conversation

bloodforcream
Copy link
Contributor

fixes #6323

SpiderLoggerAdapter prioritizes self.extra over logged extra so it works as it worked before.
spider in extra will always be self

@bloodforcream bloodforcream changed the title draft: feat: add SpiderLoggerAdapter, change Spider.logger to return SpiderLoggerAdapter feat: add SpiderLoggerAdapter, change Spider.logger to return SpiderLoggerAdapter Apr 27, 2024
Copy link

codecov bot commented Apr 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.88%. Comparing base (1d11ea3) to head (bd43233).
Report is 46 commits behind head on master.

❗ Current head bd43233 differs from pull request most recent head 1320e2a. Consider uploading reports for the commit 1320e2a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6324      +/-   ##
==========================================
+ Coverage   83.92%   88.88%   +4.95%     
==========================================
  Files         161      162       +1     
  Lines       11972    11981       +9     
  Branches     1865     1930      +65     
==========================================
+ Hits        10048    10649     +601     
+ Misses       1588      980     -608     
- Partials      336      352      +16     
Files Coverage Δ
scrapy/spiders/__init__.py 93.84% <100.00%> (+0.09%) ⬆️
scrapy/utils/spider_logger_adapter.py 100.00% <100.00%> (ø)

... and 26 files with indirect coverage changes

@bloodforcream bloodforcream changed the title feat: add SpiderLoggerAdapter, change Spider.logger to return SpiderLoggerAdapter issue #6323: add SpiderLoggerAdapter, change Spider.logger to return SpiderLoggerAdapter May 5, 2024
Copy link
Member

Choose a reason for hiding this comment

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

This should go into scrapy.utils.log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. That's where I tried to put it first but I get a circular import error.

..\scrapy\__init__.py:15: in <module>
    from scrapy.spiders import Spider
..\scrapy\spiders\__init__.py:16: in <module>
    from scrapy.utils.log import SpiderLoggerAdapter
..\scrapy\utils\log.py:13: in <module>
    from scrapy.settings import Settings
..\scrapy\settings\__init__.py:23: in <module>
    from scrapy.settings import default_settings
..\scrapy\settings\default_settings.py:322: in <module>
    USER_AGENT = f'Scrapy/{import_module("scrapy").__version__} (+https://scrapy.org)'
E   AttributeError: partially initialized module 'scrapy' has no attribute '__version__' (most likely due to a circular import)

Maybe i'm wrong and it's solvable

Copy link
Member

Choose a reason for hiding this comment

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

I still don't like having a separate module for this, I see these options:

  1. Move it to scrapy.spiders (probably no technical downsides as it's not useful anywhere else?).
  2. Remove the SpiderLoggerAdapter import from the scrapy.spiders top-level, moving it under if TYPE_CHECKING and inside def logger().
  3. (likely a worse idea) Change some other imports in the loop.

I would go with 2 in general but probably with 1 in this case.

@Gallaecio second opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done the second option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest importing SpiderLoggerAdapter on every single log call looks a bit inefficient, imho.

But it's obviously up to you guys

@wRAR
Copy link
Member

wRAR commented May 6, 2024

Is it possible to add a test that checks that the Spider logging methods work as expected?

@bloodforcream
Copy link
Contributor Author

Is it possible to add a test that checks that the Spider logging methods work as expected?

Do you mean a test that's making sure general logging messages(not "extra") of Spider work as expected?
I'll try to code them today/tomorrow

@bloodforcream
Copy link
Contributor Author

Added more tests

@bloodforcream bloodforcream requested a review from wRAR May 8, 2024 13:16
Copy link
Member

@wRAR wRAR left a comment

Choose a reason for hiding this comment

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

Thanks!

@Gallaecio Gallaecio merged commit b88f22c into scrapy:master May 13, 2024
25 checks passed
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.

Spider.logger not logging custom extra information
3 participants