Bugfix: Fixes #3763, #3830, and #4453

[simnalamburt]

Prevent --install-option and --global-option from leaking into the next dependencies in the requirements.txt file.

You should not mutate the parameters provided by the caller unless you are aware of what you're doing.

# BAD! Mutating the parameter!
def bad(param):
    param += ['a']

# OK. Binding a new object to the name.
def good(param):
    param = param + ['a']

Fixes #3763, #3830, and #4453.

Reference

• https://github.com/simnalamburt/snippets/blob/master/python/pip-4453.py

[pfmoore]

It's not at all clear why this makes a difference (I'm assuming it's something to do with in-place modification vs creating a new list?) so at a minimum, I'd want a comment explaining what's going on here. Better still would be a code change that didn't result in the reader having a huge temptation to switch back to the += form 😄

[pfmoore]

Reading the conversation thread clarifies (must read things in the right order in future :-)) but I'd still like a comment in the code.

Maybe better would be

# Take a copy so we don't mutate the value supplied by the caller
global_options = list(global_options)
global_options += self.options.get('global_options', [])

as that makes the point explicit in the code as well as in the comment.

[pradyunsg]

+1

Having a comment would definitely be helpful here. Nice catch btw.

[jlebonzec]

Hello @simnalamburt! Thanks for spotting this. 😄

If I may pitch in... If we are to take a copy, I would suggest to put it in another variable.
Currently, the global_options and install_options first refer to the arguments of the method, then refer to a modified copy of them. It can get confusing.

I would suggest something along:

        package_global_options = \
            global_options + self.options.get('global_options', [])
        package_install_options = \
            install_options + self.options.get('install_options', [])

Of course renaming in the next usages as well would be required.

I don't know if the comment should be put in that case, but I believe it is at least much less tempting to refactor into the += form with different names.

[simnalamburt]

If I may pitch in... If we are to take a copy, I would suggest to put it in another variable.

It seems to be the best of the ideas so far. I'll update the PR.

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

[pradyunsg]

Closing in favour of #5090.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.