Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce "Bundler.clean_env" #4303

Merged
merged 1 commit into from Feb 23, 2016

Conversation

Projects
None yet
6 participants
@njam
Copy link
Contributor

commented Feb 21, 2016

Via #4232
Which should return the environment as it was before bundler modified it.

Should this use "original_env" or "clean_env" or both?

At one time, the difference was that one went up one layer of env, while the other removed all Bundler values. I'm not sure if that's still true today.

You can assign this ticket to me if you want.

@indirect

This comment has been minimized.

Copy link
Member

commented Feb 16, 2016

I am okay with providing both clean_env (delete all Bundler env) and original_env (restore env to before this Bundler loaded). In Bundler 2.0, I think we should only provide original_env.

@njam

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2016

@indirect @segiddins I added original_env and clean_env (the latter with a @deprecated flag). I've also rewritten the "with_clean_env_spec" test, I think it's simpler now and covers more.

What do you think?

ENV.replace(env)
yield
ensure
ENV.replace(backup.to_hash)

This comment has been minimized.

Copy link
@segiddins

segiddins Feb 21, 2016

Member

The to_hash here is unnecessary

@segiddins

This comment has been minimized.

Copy link
Member

commented Feb 21, 2016

@RochesterinNYC want to review the spec changes?

@njam njam force-pushed the njam:original_env branch from 2f2ad49 to f2a69df Feb 21, 2016

describe "Bundler.original_env" do
it "should return the PATH present before bundle was activated" do
gemfile ""
bundle "install --path vendor/bundle"

This comment has been minimized.

Copy link
@RochesterinNYC

RochesterinNYC Feb 21, 2016

Member

These two lines seem to be present in each of the it blocks for describe "Bundler.original_env" and describe "Bundler.clean_env":

      gemfile ""
      bundle "install --path vendor/bundle"

They could be extracted into a single before block.

Bundler.with_clean_env do
expect(`echo $GEM_PATH`.strip).not_to eq(gem_path)
expect(`echo $PATH`.strip).not_to eq(path)
path = `getconf PATH`.strip + ":/foo"

This comment has been minimized.

Copy link
@RochesterinNYC

RochesterinNYC Feb 21, 2016

Member

extract these (code = and path =) and the similar = ops into let statements?

@RochesterinNYC

This comment has been minimized.

Copy link
Member

commented Feb 21, 2016

Other than a bit of possible DRYing up and more utilization of rspec features, the specs LGTM.

@njam njam force-pushed the njam:original_env branch from f2a69df to 1066a72 Feb 21, 2016

@njam

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2016

Thanks for the feedback!

I moved the "bundle install" code to a before block.
I'm not sure about let. To replace all assignments with let I would need to wrap each test in a context block where I would then define those values (code, path, result). Is it the way to go?

@RochesterinNYC

This comment has been minimized.

Copy link
Member

commented Feb 21, 2016

@njam Nope, you're right. It's possible to re-arrange these around to use let blocks but it's fine as is/isn't necessary. 👍

@segiddins

This comment has been minimized.

Copy link
Member

commented Feb 23, 2016

@homu r+

@homu

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2016

📌 Commit 1066a72 has been approved by segiddins

@homu

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2016

⌛️ Testing commit 1066a72 with merge b20fbf8...

homu added a commit that referenced this pull request Feb 23, 2016

Auto merge of #4303 - njam:original_env, r=segiddins
Introduce "Bundler.clean_env"

Via #4232
Which should return the environment as it was *before* bundler modified it.

Should this use "original_env" or "clean_env" or both?
> At one time, the difference was that one went up one layer of env, while the other removed all Bundler values. I'm not sure if that's still true today.

You can assign this ticket to me if you want.
@homu

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2016

☀️ Test successful - status

@homu homu merged commit 1066a72 into bundler:master Feb 23, 2016

3 checks passed

codeclimate Code Climate has skipped analysis of this commit.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@lynncyrin lynncyrin modified the milestone: Release Archive Sep 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.