Skip to content

build: bail early when verifying dependencies - #4915

Merged
gvanrossum merged 1 commit into
python:masterfrom
ceh:bail-early-when-verifying-deps
Apr 16, 2018
Merged

build: bail early when verifying dependencies#4915
gvanrossum merged 1 commit into
python:masterfrom
ceh:bail-early-when-verifying-deps

Conversation

@ceh

@ceh ceh commented Apr 15, 2018

Copy link
Copy Markdown
Contributor

Skip creating Options for dependency when it's in modules.

Skip creating Options for dependency when it's in modules.
@gvanrossum

Copy link
Copy Markdown
Member

What problem is this PR solving?

@ceh

ceh commented Apr 16, 2018

Copy link
Copy Markdown
Contributor Author

It's a micro-optimization, while also reducing nesting within the function. We don't need to construct Options objects for dependencies that already exists in modules.

The closest related issue is probably #4365, but this PR is not solving a specific issue.

@gvanrossum

Copy link
Copy Markdown
Member

OK, I'll merge it, but I'm not sure that random micro-optimizations like this are a good use of anybody's time. (Unless you profiled it, which I doubt you did, else you would have mentioned the speedup. :-)

@gvanrossum
gvanrossum merged commit 6a8460f into python:master Apr 16, 2018
@ceh

ceh commented Apr 16, 2018

Copy link
Copy Markdown
Contributor Author

No, I didn't profile it because I don't think it's worth the hassle.

Sorry for the noise, it's a habit of mine which helps me digest code better while trying to understand it.

Thanks for the input!

@gvanrossum

Copy link
Copy Markdown
Member

Yeah, we've had contributors in the past who seem to think that the only way for them to understand the code is to rewrite it. But there's a net cost in code churn for those who already understand it, and there's no guarantee that others will benefit (everyone's way of reading code is different).

@ceh
ceh deleted the bail-early-when-verifying-deps branch April 16, 2018 19:50
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.

2 participants