Skip to content
This repository has been archived by the owner on Jan 6, 2021. It is now read-only.

Dont change non path on win32 #107

Merged
merged 6 commits into from
Apr 13, 2017

Conversation

anaisbetts
Copy link
Contributor

Closes #106

src/variable.js Outdated
@@ -1,14 +1,26 @@
import isWindows from 'is-windows'

const pathLikeEnvVarWhitelist = ['PATH', 'NODE_PATH'].reduce(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems overly complicated. What about simply new Set(['PATH', 'NODE_PATH'])?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL

@codecov-io
Copy link

codecov-io commented Apr 13, 2017

Codecov Report

Merging #107 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #107   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          42     45    +3     
=====================================
+ Hits           42     45    +3
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️
src/variable.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6994a24...22257b2. Read the comment docs.

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great other than that one comment from @DanReyLop. Also, would you like to add yourself to the contributors table?

@kentcdodds kentcdodds changed the base branch from master to next April 13, 2017 16:36
Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. Thank you! I'm merging this into a branch called next because this is a breaking change and I'd rather group this with other breaking changes we're considering. Thanks!

"profile": "https://twitter.com/paulcbetts",
"contributions": [
"bug",
"code"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed test. You can add it in another PR if you want :)

@kentcdodds kentcdodds merged commit 8d4ec1d into kentcdodds:next Apr 13, 2017
@anaisbetts anaisbetts deleted the dont-change-non-path-on-win32 branch April 13, 2017 19:21
@anaisbetts
Copy link
Contributor Author

@kentcdodds Works for me!

kentcdodds pushed a commit that referenced this pull request May 11, 2017
* Add a whitelist for variables to : => ; ify

* Plumb the name through

* Write some tests

* Dox, linter fascism

* Much better!

* Contributors

BREAKING CHANGES: We no longer swap `:` for `;` on windows for any variables other than `PATH` and `NODE_PATH`.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants