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.

None yet

5 participants