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: check url before request #172

Merged
merged 1 commit into from
May 9, 2016
Merged

fix: check url before request #172

merged 1 commit into from
May 9, 2016

Conversation

dead-horse
Copy link
Member

否则后面会报错,让人没有头绪

> urllib.request(undefined, function(){})
TypeError: Cannot read property 'method' of undefined
    at Object.exports.requestWithCallback (/Users/deadhorse/git/urllib/lib/urllib.js:213:54)
    at Object.exports.request (/Users/deadhorse/git/urllib/lib/urllib.js:128:20)
    at repl:1:8
    at REPLServer.defaultEval (repl.js:262:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:431:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:211:10)

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @fengmk2, @xingrz and @coderhaoxin to be potential reviewers

@codecov-io
Copy link

codecov-io commented May 6, 2016

Current coverage is 98.19%

Merging #172 into master will increase coverage by +<.01%

@@             master       #172   diff @@
==========================================
  Files             1          1          
  Lines           382        386     +4   
  Methods          31         31          
  Messages          0          0          
  Branches        112        113     +1   
==========================================
+ Hits            375        379     +4   
  Misses            7          7          
  Partials          0          0          

Powered by Codecov. Last updated by d2ed15d...fadaaff

@fengmk2
Copy link
Member

fengmk2 commented May 6, 2016

每次都多了一次字符串相加…

@dead-horse
Copy link
Member Author

还好吧…损耗不大

@fengmk2
Copy link
Member

fengmk2 commented May 6, 2016

if 判断,然后抛错?

Sent from my iPhone

On May 6, 2016, at 9:20 PM, Yiyu He notifications@github.com wrote:

还好吧…损耗不大


You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub

@@ -173,6 +174,8 @@ exports.requestThunk = function (url, args) {

exports.requestWithCallback = function (url, args, callback) {
// requestWithCallback(url, callback)
assert(url && (typeof url === 'string' || url.constructor.name === 'Url'),
Copy link
Member

@fengmk2 fengmk2 May 6, 2016

Choose a reason for hiding this comment

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

if (!url || (typeof url !== 'string' && url.constructor.name !== 'Url')) {
  throw new TypeError('expect request url to be a string or an instance of Url, but got '+ url);
}

考虑到这个 request 会大量调用,能省即省。

Copy link
Member Author

Choose a reason for hiding this comment

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

好了

@fengmk2
Copy link
Member

fengmk2 commented May 7, 2016

+1

@fengmk2
Copy link
Member

fengmk2 commented May 7, 2016

@dead-horse 测试用例挂了

image

@@ -210,7 +216,7 @@ exports.requestWithCallback = function (url, args, callback) {
parsedUrl = url;
}

var method = (args.type || args.method || parsedUrl.method || 'GET').toUpperCase();
var method = (args.type || args.method || 'GET').toUpperCase();
Copy link
Member Author

Choose a reason for hiding this comment

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

规范一下,第一个参数只允许 string 或者 Url 对象可能会有不兼容

@dead-horse
Copy link
Member Author

只判断 string 或者 object 了

@dead-horse
Copy link
Member Author

感觉可以当做是一个 fix 了

@dead-horse dead-horse changed the title feat: check url before request fix: check url before request May 8, 2016
@dead-horse dead-horse added bug and removed feature labels May 8, 2016
@fengmk2
Copy link
Member

fengmk2 commented May 9, 2016

+1

@fengmk2 fengmk2 merged commit 3adc841 into master May 9, 2016
@fengmk2 fengmk2 deleted the assert branch May 9, 2016 03:04
@fengmk2
Copy link
Member

fengmk2 commented May 9, 2016

2.9.1

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.

4 participants