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

Flake8: Remove E741 #4558

Merged
merged 1 commit into from May 14, 2020
Merged

Flake8: Remove E741 #4558

merged 1 commit into from May 14, 2020

Conversation

elacuesta
Copy link
Member

@elacuesta elacuesta commented May 8, 2020

Do not use variables named 'I', 'O', or 'l'

@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #4558 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #4558      +/-   ##
==========================================
- Coverage   84.56%   84.54%   -0.02%     
==========================================
  Files         164      164              
  Lines        9923     9922       -1     
  Branches     1476     1475       -1     
==========================================
- Hits         8391     8389       -2     
  Misses       1266     1266              
- Partials      266      267       +1     
Impacted Files Coverage Δ
scrapy/shell.py 68.21% <0.00%> (ø)
scrapy/spiders/sitemap.py 80.64% <100.00%> (-0.31%) ⬇️
scrapy/utils/log.py 88.76% <100.00%> (ø)
scrapy/utils/trackref.py 82.85% <0.00%> (-2.86%) ⬇️

wRAR
wRAR approved these changes May 13, 2020
@wRAR
Copy link
Contributor

wRAR commented May 13, 2020

@elacuesta there are conflicts here

@elacuesta
Copy link
Member Author

elacuesta commented May 13, 2020

Updated, but the build will fail until we merge #4572, let's wait for that one.

b.append(" fetch(req) "
"Fetch a scrapy.Request and update local objects ")
b.append(" shelp() Shell help (print this help)")
b.append(" view(response) View response in a browser")

return "\n".join("[s] %s" % l for l in b)
return "\n".join("[s] %s" % line for line in b)
Copy link
Member

@noviluni noviluni May 13, 2020

Choose a reason for hiding this comment

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

Hi @elacuesta

Maybe it's not necessary to do it as part of this PR, but as we are giving more meaning to a lot of variable names, it could be a good idea to rename this b variable to something like banner (I assume this is the real meaning by looking at this: self.vars['banner'] = self.get_help()) or help_lines.

Copy link
Member Author

@elacuesta elacuesta May 13, 2020

Choose a reason for hiding this comment

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

Indeed, I almost did that, but I decided not to in order to keep the diff simpler. Perhaps we can do it in a later PR 👍

@elacuesta
Copy link
Member Author

elacuesta commented May 13, 2020

Rebased on top of #4572

@elacuesta elacuesta closed this May 13, 2020
@elacuesta elacuesta reopened this May 13, 2020
@elacuesta
Copy link
Member Author

elacuesta commented May 14, 2020

Updated, reverted changes to tests/test_loader.py. Will merge once CI passes.

@elacuesta elacuesta merged commit 602bb13 into scrapy:master May 14, 2020
1 of 2 checks passed
@elacuesta elacuesta deleted the flake8-remove-e741 branch May 14, 2020
@noviluni
Copy link
Member

noviluni commented Jun 25, 2020

hi @elacuesta is there something blocking to work on tests/test_loader.py? I can't understand if it was reverted because it was hard to fix conflicts or if there is any other reason

@elacuesta
Copy link
Member Author

elacuesta commented Jun 25, 2020

Hey @noviluni, it was mostly to avoid adding more changes to #4516 and the need to import the changes in itemloaders.

@noviluni
Copy link
Member

noviluni commented Jun 25, 2020

Hey @noviluni, it was mostly to avoid adding more changes to #4516 and the need to import the changes in itemloaders.

Ok, it makes sense. Thank you!

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.

None yet

4 participants