Access environment variables through Host object instead of directly.

Access environment variables through Host object instead of directly.

This makes it easier to test interactions with os.environ without
affecting or using actual environment variables in tests.

This CL doesn't change places where os.environ is used where there
is no Host object.

BUG=627887
Committed: https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761
Cr-Commit-Position: refs/heads/master@{#405680}

[qyearsley, 2016-07-13 18:24:41]

qyearsley@chromium.org changed reviewers:
+ dpranke@chromium.org, tansell@chromium.org

[Dirk Pranke, 2016-07-14 19:28:13]

Good idea, but this can be even simpler, see my suggestion below.

https://codereview.chromium.org/2143123004/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py
(right):

https://codereview.chromium.org/2143123004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py:47:
self.stderr = sys.stderr
I would add a

  self.environ = os.environ

here instead.

[qyearsley, 2016-07-14 19:52:52]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[qyearsley, 2016-07-14 19:52:58]

https://codereview.chromium.org/2143123004/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py
(right):

https://codereview.chromium.org/2143123004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py:47:
self.stderr = sys.stderr
On 2016/07/14 at 19:28:13, Dirk Pranke wrote:
> I would add a
> 
>   self.environ = os.environ
> 
> here instead.

Ah, excellent -- then the three methods below aren't necessary. This is what you
were thinking, right?

[commit-bot: I haz the power, 2016-07-14 19:53:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dirk Pranke, 2016-07-14 20:00:53]

On 2016/07/14 19:52:58, qyearsley wrote:
>
https://codereview.chromium.org/2143123004/diff/1/third_party/WebKit/Tools/Sc...
> File third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py
> (right):
> 
>
https://codereview.chromium.org/2143123004/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py:47:
> self.stderr = sys.stderr
> On 2016/07/14 at 19:28:13, Dirk Pranke wrote:
> > I would add a
> > 
> >   self.environ = os.environ
> > 
> > here instead.
> 
> Ah, excellent -- then the three methods below aren't necessary. This is what
you
> were thinking, right?

Correct. Remember it's not considered good Python style to have getters and
setters; just access the objects directly.
If you ever need to change an API in the future so that you're not accessing an
object directly, you can preserve compatibility
by using property methods.

[commit-bot: I haz the power, 2016-07-14 20:19:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 20:19:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qyearsley, 2016-07-14 22:42:59]

On 2016/07/14 at 20:00:53, dpranke wrote:
> On 2016/07/14 19:52:58, qyearsley wrote:
> >
https://codereview.chromium.org/2143123004/diff/1/third_party/WebKit/Tools/Sc...
> > File third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py
> > (right):
> > 
> >
https://codereview.chromium.org/2143123004/diff/1/third_party/WebKit/Tools/Sc...
> > third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py:47:
> > self.stderr = sys.stderr
> > On 2016/07/14 at 19:28:13, Dirk Pranke wrote:
> > > I would add a
> > > 
> > >   self.environ = os.environ
> > > 
> > > here instead.
> > 
> > Ah, excellent -- then the three methods below aren't necessary. This is what
you
> > were thinking, right?
> 
> Correct. Remember it's not considered good Python style to have getters and
setters; just access the objects directly.
> If you ever need to change an API in the future so that you're not accessing
an object directly, you can preserve compatibility
> by using property methods.

Alright; now this change has been updated after the change to make a dummy HOME
dir on linux (http://crrev.com/2141093006) was committed. Does it look good to
commit?

[Dirk Pranke, 2016-07-14 23:05:31]

lgtm

[qyearsley, 2016-07-14 23:51:02]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2016-07-14 23:51:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 02:25:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-15 02:26:02]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 02:27:49]

Description was changed from

==========
Access environment variables through Host object instead of directly.

This makes it easier to test interactions with os.environ without
affecting or using actual environment variables in tests.

This CL doesn't change places where os.environ is used where there
is no Host object.

BUG=625846
==========

to

==========
Access environment variables through Host object instead of directly.

This makes it easier to test interactions with os.environ without
affecting or using actual environment variables in tests.

This CL doesn't change places where os.environ is used where there
is no Host object.

BUG=625846

Committed: https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761
Cr-Commit-Position: refs/heads/master@{#405680}
==========

[commit-bot: I haz the power, 2016-07-15 02:27:50]

Patchset 3 (id:??) landed as
https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761
Cr-Commit-Position: refs/heads/master@{#405680}

[qyearsley, 2016-07-15 22:30:45]

Description was changed from

==========
Access environment variables through Host object instead of directly.

This makes it easier to test interactions with os.environ without
affecting or using actual environment variables in tests.

This CL doesn't change places where os.environ is used where there
is no Host object.

BUG=625846

Committed: https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761
Cr-Commit-Position: refs/heads/master@{#405680}
==========

to

==========
Access environment variables through Host object instead of directly.

This makes it easier to test interactions with os.environ without
affecting or using actual environment variables in tests.

This CL doesn't change places where os.environ is used where there
is no Host object.

BUG=627887

Committed: https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761
Cr-Commit-Position: refs/heads/master@{#405680}
==========

[mithro, 2016-07-18 01:24:16]

On 2016/07/15 02:27:50, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761
> Cr-Commit-Position: refs/heads/master@{#405680}

Generally LGTM.

My one concern here is that os.environ object is "dictionary like" rather than
being a normal dictionary. There are a couple of things which *should* work
using os.environ that don't :( - Not really sure how to fix that though.