Messages (8) msg301871 - (view) Author: Mark Byrne (Mark Byrne) * Date: 2017-09-11 10:43

To make code explicit and more readable

Use the enumerate function to replace occurrences of the pattern:

for i in range(len(sources)):
    src = sources[i]

msg301879 - (view) Author: R. David Murray (r.david.murray) * Date: 2017-09-11 15:29

Thanks for wanting to improve Python, but we don't usually accept refactoring requests like this.  We "fix" such issues when the code is touched for other reasons.  We'll see what other developers think, though.

msg301880 - (view) Author: STINNER Victor (vstinner) * Date: 2017-09-11 15:49

> Thanks for wanting to improve Python, but we don't usually accept refactoring requests like this.  We "fix" such issues when the code is touched for other reasons.  We'll see what other developers think, though.

The standard library is used by some developers are the reference style for best practices. So it makes sense to "upgrade" to code to use enumarate(). I read the short change, enumerate() usage is appropriate.

msg301894 - (view) Author: R. David Murray (r.david.murray) * Date: 2017-09-11 17:55

Anyone who uses stdlib code as examples of best practice doesn't understand the history of stdlib code.

Generally, we consider the danger of introducing bugs to be more significant than the benefit of the "cleanup" changes.  The fact that you are discussing further changes to the code in the PR make this more likely.

msg301905 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * Date: 2017-09-11 21:30

I concur with David in general. And as you can see there are questions to the concrete patch.

msg301921 - (view) Author: Mark Byrne (Mark Byrne) * Date: 2017-09-11 22:37

Generally, we consider the danger of introducing bugs to be more significant than the benefit of the "cleanup" changes.  The fact that you are discussing further changes to the code in the PR make this more likely.

I haven't updated the PR with further code changes as suggested in the PR comments because the comments don't strictly relate to the changes put forward in this issue, and I don't want to go beyond the scope of the issue - the requested changes could go in a separate PR as needed.

The motivation is to use enumerate where it is a natural fit.
If you decide not to merge, then that is ok; let me know if I can help with anything.

msg301934 - (view) Author: R. David Murray (r.david.murray) * Date: 2017-09-12 01:28

Mark: Yeah, I think my comment was directed more to haypo than you :)

msg301937 - (view) Author: Vedran Čačić (veky) * Date: 2017-09-12 05:22

Sorry, this seems like a classical example of "When all you have is a hammer, everything looks like a nail. Then, when you learn of a screwdriver, you suddenly see half of these nails as screws, completely ignoring there are other tools besides those two."

In fact, enumerate is _not_ the good choice in most of those cases, as the comments show. One should use zip, another should use ordinary iteration because the index isn't needed at all, and yet another should probably stay as it is, since that way it's more obvious that we're transforming each item in place. [When LHS is consts[i] and right is const, it's not easy to see they refer to the same place.]

Even the fourth I would argue is more readable in the current form, for a similar reason: when you base the decision on words[i], and the decision is to del words[i:], it's obvious you delete "it and everything after". Not so much in the "refactored" code.

Please don't do such things. One of Python's great strengths, especially in stdlib, is code stability. The only reason we're able to get anything done is that we have (or at least used to have) a great policy referred to in https://bugs.python.org/issue31417#msg301879, so we don't lose ourselves in enormous bikeshedding about how to best use the enormous expressivity of Python to write the code that works perfectly fine in some slightly different way.

History Date User Action Args 2022-04-11 14:58:52adminsetgithub: 75598 2017-09-12 17:27:53serhiy.storchakasetstatus: open -> closed
resolution: not a bug
stage: patch review -> resolved 2017-09-12 15:19:29rhettingersetassignee: rhettinger -> 2017-09-12 05:22:38vekysetnosy: + veky
messages: + msg301937
2017-09-12 01:28:02r.david.murraysetmessages: + msg301934 2017-09-11 22:37:08Mark Byrnesetmessages: + msg301921 2017-09-11 21:30:33serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg301905
2017-09-11 17:55:56r.david.murraysetmessages: + msg301894 2017-09-11 15:49:03vstinnersetnosy: + vstinner
messages: + msg301880
2017-09-11 15:29:36r.david.murraysetnosy: + r.david.murray
messages: + msg301879
2017-09-11 11:56:52serhiy.storchakasetassignee: rhettinger

nosy: + rhettinger

2017-09-11 10:52:01python-devsetkeywords: + patch
stage: patch review
pull_requests: + pull_request3490 2017-09-11 10:43:46Mark Byrnecreate