removed some deprecated functions and supports #6116

Chenwei-Niu · 2023-10-18T01:25:01Z

commits aabc3db and cc2ad92 resolved #6111
resolves #6109

… in 2.6.0.

… tests -- Issue scrapy#6111

codecov · 2023-10-18T07:58:16Z

Codecov Report

Merging #6116 (e5fee58) into master (9b06f6b) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head e5fee58 differs from pull request most recent head 61dfa12. Consider uploading reports for the commit 61dfa12 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6116      +/-   ##
==========================================
- Coverage   88.84%   88.80%   -0.05%     
==========================================
  Files         160      160              
  Lines       11425    11411      -14     
  Branches     1863     1859       -4     
==========================================
- Hits        10151    10133      -18     
- Misses        962      972      +10     
+ Partials      312      306       -6

Files	Coverage Δ
scrapy/utils/log.py	`91.83% <100.00%> (+3.37%)`	⬆️
scrapy/utils/response.py	`88.37% <ø> (-1.83%)`	⬇️

... and 3 files with indirect coverage changes

wRAR · 2023-10-18T08:38:35Z

Please use pre-commit locally and run tests locally, and fix the errors produced by them.

scrapy/utils/log.py

… tests -- Issue scrapy#6111

Chenwei-Niu · 2023-10-18T11:33:18Z

I think my commits are okay to rerun the workflow. @wRAR

wRAR · 2023-10-18T11:58:39Z

scrapy/utils/log.py


    level = logkws.get("level", logging.INFO)
-    message = logkws.get("format", logkws.get("msg"))
+    message = str(logkws.get("msg"))


Why str()? It wasn't needed before.

Hi, wRAR,
The problem here is that logkws.get("msg") returns Any | None instead of Any. The previous code works well since it sets a default value in get() and returns Any.
If we don't take measures here, the type of variable message is Any | None. And an incompatible return value error arises when conducting tests.

I came up with three possible solutions:

message = logkws.get("msg", logkws.get("msg"))

message = logkws.get("msg", "")

message = str(logkws.get("msg"))

Do you have any suggestions or preferences among these three solutions?

logkws.get("msg", "") is fine I think.

logkws.get("msg", "") is fine I think.

Only if msg is missing from the dict, not if it exists as None, which mypy probably considers a possibility.

If it could exist as None (e.g. it the user made a mistake), logkws.get("msg") or "" should work. Otherwise, what @wRAR said, and assert message is not None after the line should silence mypy about the impossible scenario of logkws containing {"msg": None}.

I‘ve already tested, the code below could handle both two scenarios you mentioned.

key msg is missing in the dictionary

Dictionary contains "msg": None

message = logkws.get("msg", "") assert message is not None

If there is no problem, I will modify and commit it.
@Gallaecio @wRAR

…dictionary contains 'msg': None

Gallaecio · 2023-10-20T07:25:04Z

scrapy/utils/log.py

-    if not {"level", "msg", "args"} <= set(logkws):
-        warnings.warn("Missing keys in LogFormatter method", ScrapyDeprecationWarning)
-
-    if "format" in logkws:
-        warnings.warn(
-            "`format` key in LogFormatter methods has been "
-            "deprecated, use `msg` instead",
-            ScrapyDeprecationWarning,
-        )

    level = logkws.get("level", logging.INFO)
-    message = logkws.get("format", logkws.get("msg"))
+    message = logkws.get("msg", "")
+    assert message is not None


OK, now that I look at the warning, I see 2 things:

The warning was about msg being a required key, so I think logkws["msg"] instead of logkws.get("msg", "") makes sense now that the warning is gone.

There was no warning if logkws["msg"] was None, so we should assume that as a possibility since this dict seems to be user-provided. So we cannot assert, we have to fall back: message = logkws["msg"] or "".

Hi Gallarcio,
I don't really understand why we have to fall back: message = logkws["msg"] or "".

Firstly ,this part of the warnings has been removed according to @wRAR 's comments #6116 (comment)

if not {"level", "msg", "args"} <= set(logkws): warnings.warn("Missing keys in LogFormatter method", ScrapyDeprecationWarning)

Secondly:

Gallaecio: The warning was about msg being a required key, so I think logkws["msg"] instead of logkws.get("msg", "") makes sense now that the warning is gone.

Although msg is a requried key, but it is only mentioned and constrained in comments in class LogFormatter, there is no actual code constraints. The user can still provide a dictionary without the msg key. Hence, if key msg is missing, simply using logkws["msg"] would arise a KeyError.

And I agree the point we cannot assert message is not None.
In fact, our current logging system could cope with the case {"msg": None}. So maybe only message = logkws.get("msg", "") should be fine.

I think raising KeyError is fine. I think it is the natural evolution from the warning: first msg was probably not necessary, then it became necessary but it worked without it causing a warning, now it will raise KeyError if missing.

If "msg": None is OK, then I agree the fall back is not needed, and we can simply use logkws["msg"].

By using or operator in case we have the key msg but the value is null we fallback to empty string, since the default empty string value using get() is only when the dict don’t have the key not for the key being null.

Probably the best solution for this is would be:

message = logkws.get("msg") or ""

Unless like @Gallaecio said we want to raise a exception so the user can fix their code, then we don’t need to get at all only the or operator to handle cases where key exist but is null.

Hi @Laerte
I finally edited to message = logkws.get("msg") or "" as your suggestion.
Could you please review and merge my pull request?

Thank you!

I don’t have write access, we need to wait for @wRAR / @Gallaecio

Hi @wRAR @Gallaecio , could you please review and merge this pull request?
Thanks!

Gallaecio · 2023-10-30T08:59:21Z

Thanks everyone!

Chenwei-Niu added 2 commits October 18, 2023 12:17

removed Deprecated function 'scrapy.utils.response.response_httprepr'…

aabc3db

… in 2.6.0.

Remove deprecated support from scrapy.utils.log.logformatter_adapter()

01b5756

Chenwei-Niu changed the title ~~removed Deprecated function scrapy.utils.response.response_httprepr~~ removed some deprecated functions and supports Oct 18, 2023

remove the occurrences of function response_httprepr in corresponding…

cc2ad92

… tests -- Issue scrapy#6111

wRAR reviewed Oct 18, 2023

View reviewed changes

scrapy/utils/log.py Outdated Show resolved Hide resolved

Chenwei-Niu and others added 3 commits October 18, 2023 21:32

remove the occurrences of function response_httprepr in corresponding…

1ac3007

… tests -- Issue scrapy#6111

Merge branch 'master' of https://github.com/Chenwei-Niu/scrapy

0ec702b

Merge branch 'scrapy:master' into master

9197ca1

Chenwei-Niu requested a review from wRAR October 18, 2023 11:34

wRAR reviewed Oct 18, 2023

View reviewed changes

Chenwei-Niu added 2 commits October 20, 2023 09:31

Handle two cases: 1. key msg is missing in the dictionary\n 2.logkws …

787df66

…dictionary contains 'msg': None

Merge branch 'master' of https://github.com/Chenwei-Niu/scrapy

aae0590

Chenwei-Niu requested review from Gallaecio and wRAR October 19, 2023 22:33

Gallaecio reviewed Oct 20, 2023

View reviewed changes

Chenwei-Niu requested a review from Gallaecio October 20, 2023 09:12

using 'or' operator to fallback to empty string and remove the assertion

61dfa12

Chenwei-Niu requested a review from Laerte October 20, 2023 10:09

Laerte approved these changes Oct 26, 2023

View reviewed changes

Gallaecio merged commit 1f797d0 into scrapy:master Oct 30, 2023
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removed some deprecated functions and supports #6116

removed some deprecated functions and supports #6116

Chenwei-Niu commented Oct 18, 2023 •

edited

codecov bot commented Oct 18, 2023 •

edited

wRAR commented Oct 18, 2023

Chenwei-Niu commented Oct 18, 2023 •

edited

wRAR Oct 18, 2023

Chenwei-Niu Oct 19, 2023

wRAR Oct 19, 2023

Gallaecio Oct 19, 2023

Chenwei-Niu Oct 19, 2023 •

edited

Gallaecio Oct 20, 2023

Chenwei-Niu Oct 20, 2023

Gallaecio Oct 20, 2023

Laerte Oct 20, 2023 •

edited

Laerte Oct 20, 2023

Laerte Oct 20, 2023 •

edited

Chenwei-Niu Oct 23, 2023

Laerte Oct 23, 2023

Chenwei-Niu Oct 25, 2023 •

edited

Gallaecio commented Oct 30, 2023

removed some deprecated functions and supports #6116

removed some deprecated functions and supports #6116

Conversation

Chenwei-Niu commented Oct 18, 2023 • edited

codecov bot commented Oct 18, 2023 • edited

Codecov Report

wRAR commented Oct 18, 2023

Chenwei-Niu commented Oct 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chenwei-Niu Oct 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laerte Oct 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laerte Oct 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chenwei-Niu Oct 25, 2023 • edited

Choose a reason for hiding this comment

Gallaecio commented Oct 30, 2023

Chenwei-Niu commented Oct 18, 2023 •

edited

codecov bot commented Oct 18, 2023 •

edited

Chenwei-Niu commented Oct 18, 2023 •

edited

Chenwei-Niu Oct 19, 2023 •

edited

Laerte Oct 20, 2023 •

edited

Laerte Oct 20, 2023 •

edited

Chenwei-Niu Oct 25, 2023 •

edited