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+2] [logformatter] 'flags' format spec backward compatibility #2649

Merged
merged 2 commits into from
Mar 21, 2017

Conversation

pawelmhm
Copy link
Contributor

pass 'flags' kwarg to logger so that it is compatible with old format of CRAWLEDMSG. fixes #2647

pass 'flags' kwarg to logger so that it is compatible with old
format of CRAWLEDMSG.
@pawelmhm pawelmhm force-pushed the logformatter-2647 branch from 0c5f9a6 to 0f2a5cd Compare March 13, 2017 14:16
@redapple redapple added this to the v1.4 milestone Mar 14, 2017
@redapple redapple changed the title [logformatter] 'flags' format spec backward compatibility [MRG+1] [logformatter] 'flags' format spec backward compatibility Mar 14, 2017
@@ -43,6 +43,7 @@ def crawled(self, request, response, spider):
'request_flags' : request_flags,
'referer': referer_str(request),
'response_flags': response_flags,
'flags': response_flags
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a comment which says that flags alias is for backwards compatibility with Scrapy < 1.4?

@@ -64,5 +64,32 @@ def test_scraped(self):
assert all(isinstance(x, six.text_type) for x in lines)
self.assertEqual(lines, [u"Scraped from <200 http://www.example.com>", u'name: \xa3'])


class LogFormatterSubclass(LogFormatter):
# Formatter with format spec that is same as in Scrapy before 1.3 version.
Copy link
Member

@kmike kmike Mar 14, 2017

Choose a reason for hiding this comment

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

I think response.flags will be released as a part of 1.4, not 1.3.x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comments updated

@codecov-io
Copy link

Codecov Report

Merging #2649 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2649      +/-   ##
==========================================
+ Coverage   84.25%   84.26%   +0.01%     
==========================================
  Files         162      162              
  Lines        9106     9106              
  Branches     1350     1350              
==========================================
+ Hits         7672     7673       +1     
  Misses       1174     1174              
+ Partials      260      259       -1
Impacted Files Coverage Δ
scrapy/logformatter.py 90.47% <ø> (ø)
scrapy/utils/trackref.py 86.48% <0%> (+2.7%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfb5640...4345eaf. Read the comment docs.

@kmike
Copy link
Member

kmike commented Mar 20, 2017

Thanks @pawelmhm, looks good to me!

@kmike kmike changed the title [MRG+1] [logformatter] 'flags' format spec backward compatibility [MRG+2] [logformatter] 'flags' format spec backward compatibility Mar 20, 2017
@redapple redapple merged commit 776129a into scrapy:master Mar 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After adding request flags subclasses of logformatter that rely on 'flags' format string are broken
4 participants