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

fix(config): Use boolifyWithDefault() for bools from environment #4823

Merged
merged 1 commit into from
Nov 2, 2017
Merged

fix(config): Use boolifyWithDefault() for bools from environment #4823

merged 1 commit into from
Nov 2, 2017

Conversation

sth
Copy link
Contributor

@sth sth commented Nov 1, 2017

Summary

Use boolifyWithDefault() to determine if environment variable values are true or false. This ensures that all environment variables interpret the same values the same way.

This changes the behavior of YARN_SILENT and YARN_IGNORE_PATH if they have "unexpected" values, all nonempty stings beside "0" and "false" are now interpreted as true. For example YARN_SILENT=hello was interpreted as false before, now it is true. This makes some sense since those values are truthy in Javascript, but generally makes things more predictable if it works like that for all yarn environment variables.

YARN_SILENT=true was also interpreted as false. This now definitely makes more sense since it will be interpreted as true.

See also #4811.

Test plan

There should be no change to the existing intended functionality and the existing tests still pass.

@buildsize
Copy link

buildsize bot commented Nov 1, 2017

This change will decrease the build size from 9.94 MB to 9.94 MB, a decrease of 912 bytes (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 860.23 KB 860.06 KB -175 bytes (0%)
yarn-[version].js 3.78 MB 3.78 MB -264 bytes (0%)
yarn-legacy-[version].js 3.84 MB 3.83 MB -264 bytes (0%)
yarn-v[version].tar.gz 865.46 KB 865.37 KB -97 bytes (0%)
yarn_[version]all.deb 654.83 KB 654.72 KB -112 bytes (0%)

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Awesome!

There should be no change to the existing intended functionality and the existing tests still pass.

I'd say boolifyWithDefault deserves some unit tests at this point but may be we can do it in a follow-up?

@BYK BYK changed the title Use boolifyWithDefault() for bools from environment fix(config): Use boolifyWithDefault() for bools from environment Nov 2, 2017
@BYK BYK merged commit c2b43b6 into yarnpkg:master Nov 2, 2017
@sth sth deleted the boolifyWithDefault branch November 2, 2017 20:44
calvinhuang pushed a commit to calvinhuang/yarn that referenced this pull request Nov 9, 2017
…npkg#4823)

**Summary**

Use `boolifyWithDefault()` to determine if environment variable values are `true` or `false`. This ensures that all environment variables interpret the same values the same way.

This changes the behavior of `YARN_SILENT` and `YARN_IGNORE_PATH` if they have "unexpected" values, all nonempty stings beside `"0"` and `"false"` are now interpreted as `true`. For example `YARN_SILENT=hello` was interpreted as `false` before, now it is `true`. This makes some sense since those values are truthy in Javascript, but generally makes things more predictable if it works like that for all yarn environment variables.

`YARN_SILENT=true` was also interpreted as `false`. This now definitely makes more sense since it will be interpreted as `true`.

See also [yarnpkg#4811](yarnpkg#4811 (comment)).

**Test plan**

There should be no change to the existing intended functionality and the existing tests still pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants