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.

Sign up for GitHub

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

Jump to bottom

Allow empty strings. #4

Merged
merged 4 commits into from Dec 20, 2016
Merged

Allow empty strings. #4

merged 4 commits into from Dec 20, 2016

Conversation

jacobtolar
Copy link
Contributor

Before this change, you get an error if you've explicitly set a value to empty and later try to retrieve it.

@jacobtolar
Copy link
Contributor Author

The build failure seems to be due to dependency version changes?

If I pin request to 2.75.0 all tests pass. On 2.76.0 a few fail. Not sure what the deeper issue is...

request/request@v2.75.0...v2.76.0

Installing 2.76.0 dep from 2.75.0 gives me:

- core-util-is@1.0.2 node_modules/core-util-is
- isarray@1.0.0 node_modules/isarray
- process-nextick-args@1.0.7 node_modules/process-nextick-args
- string_decoder@0.10.31 node_modules/string_decoder
- util-deprecate@1.0.2 node_modules/util-deprecate
- readable-stream@2.0.6 node_modules/readable-stream
- bl@1.1.2 node_modules/bl
shell-variables@1.2.0 /src/shell-variables
└─┬ request@2.76.0
  ├── form-data@2.1.2
  └── qs@6.3.0

@jacobtolar
Copy link
Contributor Author

Found it; request/request#2431 changes the timeout to happen on socket connect, not request.start()

Updated the request dependency to include this change, and updated nock to be able to use the socket timeout feature.

stjohnjohnson
stjohnjohnson requested changes Dec 19, 2016
View reviewed changes
lib/client.js
@@ -14,7 +14,7 @@ function getValue (field, callback) {
timeout: this.timeout,
json: true
}, function (error, response, body) {
if (!error && response.statusCode === 200 && body.value) {
if (!error && response.statusCode === 200 && (body.value || body.value === "")) {
Copy link
Owner

Choose a reason for hiding this comment

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

Didn't realize this, doesn't this mean 0 and false are also going to fail? Can we change this to body.value !== undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that makes sense.

package.json
@@ -25,11 +25,11 @@
"dependencies": {
"get-stdin": "^0.1.0",
"nomnom": "^1.8.0",
"request": "^2.37.0"
"request": "2.76.0"
Copy link
Owner

Choose a reason for hiding this comment

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

So the intention is to pin to 2.76 now? Just to be clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, whoops. That was from some testing. I'll change it to '^2.76.0'.

@stjohnjohnson
Copy link
Owner

Sorry for the delay, just some quick questions.

stjohnjohnson
stjohnjohnson approved these changes Dec 20, 2016
View reviewed changes
Copy link
Owner

@stjohnjohnson stjohnjohnson left a comment

Choose a reason for hiding this comment

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

yes

@stjohnjohnson stjohnjohnson merged commit edb9eda into stjohnjohnson:master Dec 20, 2016
@stjohnjohnson
Copy link
Owner

  • shell-variables@1.3.0

@jacobtolar
Copy link
Contributor Author

thanks!

@jacobtolar jacobtolar deleted the allow-empty-string branch December 23, 2016 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stjohnjohnson stjohnjohnson stjohnjohnson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jacobtolar @stjohnjohnson