Add Popen_with_deplayed_expansion to process_utils

[uvdn7]

On Windows, by default, environment variables are expanded before execution. This behavior is not what we want. Popen_with_deplayed_expansion is a thin wrapper around subprocess.Popen which tweaks the incoming cmd a little bit, which makes sure the environment variables will be expanded during execution, before passing it to the subprocess.Popen.

[boxcla]

Verified that @aptxkid has signed the CLA. Thanks for the pull request!

[josephharrington]

Do you think we would want to use this wrapper everywhere throughout the app? If so, we might want to just name it Popen and maybe enforce that this wrapper must be used instead of subprocess.Popen directly.

[uvdn7]

1. I can't think of a reason not to use it everywhere throughout the app
2. You mean monkey patch subprocess.Popen at the very beginning during import? Can you elaborate how exactly you want to enforce it?

[josephharrington]

Probably not patching. We'd probably enforce via convention (lightest weight), custom lint rule, or unit test blacklist.

I'm not too concerned about the enforcing since we very rarely add Popen calls. The main thing I was wondering is if we should just name this Popen.

[uvdn7]

@josephharrington I don't like to name it Popen as it's confusing to people. Why I should use this Popen instead of subprocess.Popen? Naming it this way, IMO, is explicit so hopefully more readable and maintainable. I am going to merge this as is but I am more than happy to change it later. 😃

[josephharrington]

I totally agree that naming it something more descriptive would be helpful to people encountering it for the first time.

My main reasoning about why I'd name it something other than Popen_with_delayed_expansion specifically is that I can totally see us adding more functionality to this method in the future, especially if, as you said, we'd probably want to use it everywhere throughout the app. (Just as an example of what I mean: if we add some sort of output redirection, then Popen_with_delayed_expansion would either become Popen_with_delayed_expansion_and_output_redirection or more likely just become a not-quite-so-descriptive name.) I suppose something still descriptive but a bit more general such as "Popen_cross_platform" would avoid that.

I also think the naming discussion definitely depends on how this will be used. More specifically, I think it depends on if it's more of a special case when we do use this instead of standard Popen or when we don't. (And actually I think the answer to that is pretty tightly linked to the likelihood that we'll add more functionality to this method in the future, so this is pretty close to my previous point.)

Answering the question of "Why I should use this Popen instead of subprocess.Popen?" was also what I was thinking about when suggesting something like a lint rule. Chances are that even with the name Popen_with_delayed_expansion someone will still end up going to read the method's docstring to find out what delayed expansion is anyway. :) We'd probably want a lint rule if we decided there would never be a reason someone would not want to use this -- but I'm not yet sure that's true.

I'm fine with leaving this for now -- we can always come back to it if we want to. I just mostly wanted to explain my line of reasoning in excruciating detail. If you've read this far then you've suffered enough. 😪

[uvdn7]

lol. It's all good thoughts!

[josephharrington]

Small comment about naming that you can ignore if you want to. 😄 Otherwise, 👍 .