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

Remove env variables that don't exist from the converted commands in Windows. #149

Merged
merged 5 commits into from
Oct 27, 2017

Conversation

DanReyLop
Copy link
Contributor

Fixes #145, check it for more context.

What: Remove env variables that don't exist from the converted commands in Windows.

Why: In Windows, if a %variable% is not defined (or it's the empty string), the shell won't parse it, and instead it will interpret it as the literal %variable% string. In UNIX, an undefined or empty variable wuld be interpreted as an empty string.

How: The entire process.env map that would be passed down to the child process is now also passed to the commandConvert function. While converting a env variable to the Windows format ($var => %var%), it's checked if it actually has any value, and if it isn't, it's ommitted entirely.

Checklist:

  • Documentation N/A
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@DanReyLop DanReyLop self-assigned this Oct 23, 2017
@DanReyLop DanReyLop changed the title Fix undefined vars windows Remove env variables that don't exist from the converted commands in Windows. Oct 23, 2017
@codecov
Copy link

codecov bot commented Oct 23, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #149   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          54     57    +3     
  Branches       11     13    +2     
=====================================
+ Hits           54     57    +3
Impacted Files Coverage Δ
src/command.js 100% <100%> (ø) ⬆️
src/index.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 36b009e...5276a75. 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.

This looks great to me. Thank you @DanReyLop!

Let's get another maintainer to review this 👍

Copy link
Collaborator

@hgwood hgwood left a comment

Choose a reason for hiding this comment

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

Looks good to me. :)

@kentcdodds kentcdodds merged commit 50299d9 into kentcdodds:master Oct 27, 2017
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #149   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          54     57    +3     
  Branches       11     13    +2     
=====================================
+ Hits           54     57    +3
Impacted Files Coverage Δ
src/command.js 100% <100%> (ø) ⬆️
src/index.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 36b009e...5276a75. Read the comment docs.

DanReyLop pushed a commit to Automattic/wp-calypso that referenced this pull request Oct 27, 2017
…mmands in Windows with undefined env variables

Fixes #16452
Closes #19084

The latest version of `cross-env` (`5.1.1`) includes a fix to #16452.
The fix is: kentcdodds/cross-env#149
Now the behaviour of commands like `cross-env node $UNDEFINED_VAR index.js` is expanding the variable
to the empty string, both in Windows and in UNIX.

To get the new version of `cross-env` working, I had to change all `cross-env` commands to `cross-env-shell`,
as this was a breaking change on `cross-env@5`.

To test: run `npm start` on a Windows `cmd.exe` shell, check that it works.

cc/ @Viper007Bond @withinboredom
DanReyLop pushed a commit to Automattic/wp-calypso that referenced this pull request Nov 2, 2017
…ommands in Windows with undefined env variables

Fixes #16452
Closes #19084

The latest version of `cross-env` (`5.1.1`) includes a fix to #16452.
The fix is: kentcdodds/cross-env#149
Now the behaviour of commands like `cross-env node $UNDEFINED_VAR index.js` is expanding the variable
to the empty string, both in Windows and in UNIX.

To get the new version of `cross-env` working, I had to change all `cross-env` commands to `cross-env-shell`,
as this was a breaking change on `cross-env@5`.

To test: run `npm start` on a Windows `cmd.exe` shell, check that it works.

cc/ @Viper007Bond @withinboredom
DanReyLop pushed a commit to Automattic/wp-calypso that referenced this pull request Nov 16, 2017
…ommands in Windows with undefined env variables

Fixes #16452
Closes #19084

The latest version of `cross-env` (`5.1.1`) includes a fix to #16452.
The fix is: kentcdodds/cross-env#149
Now the behaviour of commands like `cross-env node $UNDEFINED_VAR index.js` is expanding the variable
to the empty string, both in Windows and in UNIX.

To get the new version of `cross-env` working, I had to change all `cross-env` commands to `cross-env-shell`,
as this was a breaking change on `cross-env@5`.

To test: run `npm start` on a Windows `cmd.exe` shell, check that it works.

cc/ @Viper007Bond @withinboredom
DanReyLop pushed a commit to Automattic/wp-calypso that referenced this pull request Nov 17, 2017
…ommands in Windows with undefined env variables

Fixes #16452
Closes #19084

The latest version of `cross-env` (`5.1.1`) includes a fix to #16452.
The fix is: kentcdodds/cross-env#149
Now the behaviour of commands like `cross-env node $UNDEFINED_VAR index.js` is expanding the variable
to the empty string, both in Windows and in UNIX.

To get the new version of `cross-env` working, I had to change all `cross-env` commands to `cross-env-shell`,
as this was a breaking change on `cross-env@5`.

To test: run `npm start` on a Windows `cmd.exe` shell, check that it works.

cc/ @Viper007Bond @withinboredom
This was referenced Sep 13, 2024
This was referenced Sep 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-existing env vars break the commands in Windows
4 participants