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

Fixes #7561: Use Scandir to speed up our os.walk usage #7712

Merged
merged 2 commits into from Mar 5, 2018

Conversation

CuriousLearner
Copy link
Contributor

Used scandir instead of os.walk to speed up usage of walk method.
I've imported it as scandir_walk to explicitly keep name-spacing consistent. This would help people in future to quickly know that it is not normal walk that is shipped with os module.

Please delete anything that isn't relevant to your patch.

  • This PR relates to an existing open issue and there are no existing
    PRs open for the same issue.
  • Add Fixes #ISSUENUM at the top of your PR.
  • Add a description of the the changes introduced in this PR.

Let me know if this needs any further changes ;)

Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

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

The pull request template is to show you what kind of thing you have in the pull request - you don't need to include it directly.

Also, the instruction to do "Add Fixes #ISSUENUM at the top of your PR." means literally at the top of the comments - github doesn't parse it if you write it in the summary field.

@@ -40,6 +40,7 @@
import pytz

from babel import Locale
from scandir import walk as scandir_walk
Copy link
Member

Choose a reason for hiding this comment

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

just do import scandir so you can replace os... with scandir... all over.

@CuriousLearner
Copy link
Contributor Author

@eviljeff Done :)

Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

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

great, thanks

@eviljeff eviljeff merged commit 8e4e8c3 into mozilla:master Mar 5, 2018
@CuriousLearner
Copy link
Contributor Author

@eviljeff Not sure why the circle CI is failing on master and showing that scandir wasn't found and could not be imported. I see that it is already part of requirements in dev.txt. Can you please look into this?

@eviljeff
Copy link
Member

eviljeff commented Mar 6, 2018

Ah! Good call, the requirement should in prod.txt instead.

@CuriousLearner
Copy link
Contributor Author

Ah, sorry, I didn't know that. I'm sending a patch right away :)

@eviljeff
Copy link
Member

eviljeff commented Mar 6, 2018

Thanks! (I didn't remember either - and Travis passing fooled me)

@CuriousLearner
Copy link
Contributor Author

@eviljeff Here is the new PR: #7730

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.

None yet

2 participants