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

remove .keys() to avoid creating a tmp list/keyview obj #4031

Merged
merged 2 commits into from
Sep 27, 2019

Conversation

s-sanjay
Copy link
Contributor

This is a small straight forward refactor to avoid creating a list or keyview object ( list in 2.x and key in 3.x ).
There are other folders where this change can be made but starting with commands folder.

@s-sanjay
Copy link
Contributor Author

the test seems to only fail in py35-pinned env... weird..

@codecov
Copy link

codecov bot commented Sep 22, 2019

Codecov Report

Merging #4031 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master    #4031   +/-   ##
=======================================
  Coverage   85.67%   85.67%           
=======================================
  Files         165      165           
  Lines        9726     9726           
  Branches     1461     1462    +1     
=======================================
  Hits         8333     8333           
  Misses       1136     1136           
  Partials      257      257
Impacted Files Coverage Δ
scrapy/commands/parse.py 20.21% <0%> (-0.12%) ⬇️
scrapy/commands/runspider.py 75.36% <0%> (+1.07%) ⬆️

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

The 2nd and 3rd changes seem straightforward to me, but I wonder what is the impact of the first change (max_level) in performance.

@s-sanjay
Copy link
Contributor Author

The 2nd and 3rd changes seem straightforward to me, but I wonder what is the impact of the first change (max_level) in performance.

so performance wise we are avoiding creating a dynamic list of size of both the list.
we loop through both the dict, and take the max and take max of that max.
earlier we loop through both the dict and create combined new list of keys and then find max in that list.

@s-sanjay
Copy link
Contributor Author

@Gallaecio can this be merged ?

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

We’ll need at least 1 more approval before merging.

@Gallaecio Gallaecio merged commit 2c14692 into scrapy:master Sep 27, 2019
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.

3 participants