Place Content-encoding header into response's flags is desired

[T0shik]

Fixes #1988

Continuation of #4025 due to abandonment.

additional changes:

• 'tests/ignores.txt' in conftest.py to rely on Path
• tests to use unittest assertions
• remove create_spider_mw and make config test specific (can change this if needed)

Resolves #4025

[Gallaecio]

Thank you for reviving these changes!

Most of my feedback is probably about code you might not even have written yourself, and feedback that I failed to provide to the previous author, so apologies beforehand for that.

[T0shik]

in addition to the previous comments:

• fixed inconsistency with having COMPRESSION_KEEP_ENCODING_HEADER and COMPRESSION_KEEP_ENCODING_HEADERS
• HttpCompressionMiddleware.process_response to always return b'decoded' in decoding happened. not when keep_encoding_header is True

[wRAR]

Please note that the tests failed.

[elacuesta]

My concern is similar to the one I had before. In this case, the "decoded" flag does not actually say which was the value of the original header.
Since I wrote the above comment, #1877 was solved, so I think now an alternative would be to add a response metadata attribute to store this value (or at a minimum, store content-encoding=<value> as a flag).

Furthermore, I see no reason for the flag to be bytes, existing flags are all str.

[codecov]

Codecov Report

Merging #5290 (ed157d5) into master (9b8285d) will increase coverage by 0.02%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master    #5290      +/-   ##
==========================================
+ Coverage   88.56%   88.58%   +0.02%     
==========================================
  Files         163      163              
  Lines       10635    10648      +13     
  Branches     1812     1815       +3     
==========================================
+ Hits         9419     9433      +14     
  Misses        939      939              
+ Partials      277      276       -1     

Impacted Files	Coverage Δ
scrapy/downloadermiddlewares/httpcompression.py	92.50% <93.33%> (-0.15%)	⬇️
scrapy/settings/default_settings.py	98.75% <100.00%> (+<0.01%)	⬆️
scrapy/core/downloader/__init__.py	92.48% <0.00%> (+1.50%)	⬆️

[T0shik]

I couldn't figure out the previous build error
https://github.com/scrapy/scrapy/runs/4026144087?check_suite_focus=true

docs/conftest.py:24: in <module>
    CodeBlockParser(future_imports=['print_function']),
E   TypeError: __init__() got an unexpected keyword argument 'future_imports'

I've amended my changes to touch less files, while in there I spotted the mutation on the original response content encoding array and decided to remove it.

I think if the following build fails in a similar fashion I'll need a hand

[Gallaecio]

I couldn't figure out the previous build error https://github.com/scrapy/scrapy/runs/4026144087?check_suite_focus=true

docs/conftest.py:24: in <module>
    CodeBlockParser(future_imports=['print_function']),
E   TypeError: __init__() got an unexpected keyword argument 'future_imports'

#5299

[T0shik]

@Gallaecio thank you, will keep an eye out for when that's merged

[Gallaecio]

Closing and reopening for a fresh CI run.

[Gallaecio]

@T0shik Thanks again for your work. I am very sorry it took me so long to get back to this.

@elacuesta

My concern is similar to the one I had before. In this case, the "decoded" flag does not actually say which was the value of the original header. Since I wrote the above comment, #1877 was solved, so I think now an alternative would be to add a response metadata attribute to store this value (or at a minimum, store content-encoding=<value> as a flag).

I don’t think we need this, since after this change it is possible to access the original header.

Furthermore, I see no reason for the flag to be bytes, existing flags are all str.

While I agree, I think we should keep it as bytes for backward compatibility. We could add it as both, and deprecate somehow the bytes version (wrapping the set to detect access by key?), but I think that would be a non-trivial change and I think it may be best to keep it out of this pull request.

[Gallaecio]

It would be nice to add stacklevel=2 to the first and last warn() calls, so that the warning becomes about the super().__init__() line that is missing the corresponding parameters.

Also, I don’t agree with what @elacuesta suggested earlier about changing the warning message in the middle. Precisely because of what he explains between parenthesis (“when not passing settings at all also stops being supported, I guess”), I find the current message confusing, because a user that did not set the setting to any value will now be told not to set it to False, which the user is already not doing. I think the previous message was better. Maybe we could give it additional context for clarification instead:

                warnings.warn(
                    "The default value of COMPRESSION_KEEP_ENCODING_HEADER, "
                    "False, is deprecated, and will stop working and stop "
                    "being its default value in a future version of Scrapy. "
                    "Set COMPRESSION_KEEP_ENCODING_HEADER=True in your "
                    "settings to remove this warning.",
                    ScrapyDeprecationWarning,
                    stacklevel=2,
                )

[Gallaecio]

This should be False for backward compatibility. It should only be set to True in settings.py.tmpl.

Accordingly, we modify self.assertIn(b'gzip', newresponse.headers['Content-Encoding']) and similar in the non-setting tests to use assertNotIn instead and remove the follow-up line that checks the header value.