-
-
Notifications
You must be signed in to change notification settings - Fork 717
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
Feature/encoder decoder #160
Conversation
@@ -1,4 +1,6 @@ | |||
language: node_js | |||
env: | |||
- CXX=g++-4.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this just needed for the iconv
dev dep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
This looks great! (pending comments) bumping the linter complexity requirements are fine for this. |
9024793
to
60e326d
Compare
Re-pushed squashed commits that use |
60e326d
to
228107b
Compare
}); | ||
|
||
t.test('throws error with wrong decoder', function (st) { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of try/catch, you can use t.throws(function () { … }, new TypeError('Decoder has to be a function.'))
here
228107b
to
f91a1e5
Compare
Pushed squashed commits that changed Error → TypeError and used st.throws instead of an try catch. |
This PR adds a
decoder
andencoder
option for querystrings. These option are important when dealing with servers that process data in a form other thanutf8
. Namely when trying to send a request to Servers that useshift_jis
as method this is an important feature.Note: I choose to add an option instead of re-using the
encode
option to only require semver-minor. Also I bumped the limit for the linting because I couldn't figure out how to push this change more elegantly.