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

lib/https: convert to using internal/errors #15603

Closed
wants to merge 1 commit into from

Conversation

@ramimoshe
Copy link
Contributor

@ramimoshe ramimoshe commented Sep 25, 2017

covert lib/https.js over to using lib/internal/errors.js

ref: #11273

doc/api/errors.md Outdated
### ERR_HTTP_NO_DOMAIN

An error using the `'ERR_HTTP_NO_DOMAIN'` code is thrown specifically when an attempt
is made to parse an URL without a valid domain name.

This comment has been minimized.

@lpinca

lpinca Sep 26, 2017
Member

Tiny nit: "a URL".

@lpinca
lpinca approved these changes Sep 26, 2017
@lpinca lpinca mentioned this pull request Sep 27, 2017
4 tasks done
@ramimoshe
Copy link
Contributor Author

@ramimoshe ramimoshe commented Sep 27, 2017

Hi, who is responsible to merge this pr?

@lpinca
Copy link
Member

@lpinca lpinca commented Sep 27, 2017

@ramimoshe any collaborator can land a PR but we wait for 48+ h to make sure everyone had a chance to look at it.

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 28, 2017

Is this code currently not covered? In that case it would be great to add a new test.

@BridgeAR BridgeAR requested a review from nodejs/tsc Sep 28, 2017
Copy link
Member

@BridgeAR BridgeAR left a comment

Please use the already existing error type ERR_INVALID_DOMAIN_NAME instead of adding a redundant new one.

Copy link
Member

@BridgeAR BridgeAR left a comment

LGTM even though I would strongly prefer to have a test added for this case before landing.

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 28, 2017

@ramimoshe would you be so kind and add a test for this as well? That would be really great. If you need any help, please let me know.

Copy link
Member

@indutny indutny left a comment

LGTM

@Trott
Copy link
Member

@Trott Trott commented Sep 29, 2017

@BridgeAR This code is indeed covered in tests. test/parallel/test-http-invalid-urls.js: will exercise it. Unfortunately, that test appears buggy. It makes the perfectly reasonable but incorrect assumption that if the second argument in assert.throws() is a string, then it corresponds to the expected message. Alas, no. (This gotcha is documented.)

IMO, this PR should fix that test. This line:

const error = 'Unable to determine the domain name';

...should be changed to something like this:

const error = common.expectsError({code: 'ERR_INVALID_DOMAIN_NAME'});

(Maybe include the message property too? Maybe change the name from error to something like expectedError? But those improvements can happen subsequently, if at all.)

@Trott
Copy link
Member

@Trott Trott commented Sep 29, 2017

(Also, this will need a CI run before landing, of course.)

@Trott
Copy link
Member

@Trott Trott commented Sep 29, 2017

(Actually, I'm inclined to CI this now, land it, and then PR the test fix myself. If @ramimoshe wants to learn all about how we write our tests and stuff, awesome. But I'm going to avoid piling on changes for a first-time contribution.)

@Trott Trott force-pushed the ramimoshe:https_use_internal_errors branch to b093e7d Sep 29, 2017
@Trott
Copy link
Member

@Trott Trott commented Sep 29, 2017

Hmmm....merge conflicts even though GitHub doesn't seem to notice them.

Squashed, rebased, force-pushed. Trying again.

CI: https://ci.nodejs.org/job/node-test-pull-request/10329/

@Trott
Copy link
Member

@Trott Trott commented Sep 29, 2017

Landed in 4843c2f

@Trott Trott closed this Sep 29, 2017
Trott added a commit that referenced this pull request Sep 29, 2017
PR-URL: #15603
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@Trott
Copy link
Member

@Trott Trott commented Sep 29, 2017

Welcome @ramimoshe and thanks for the contribution! 🎉

@ramimoshe
Copy link
Contributor Author

@ramimoshe ramimoshe commented Sep 29, 2017

Thanks everyone :)
Why did you close the PR without "merge"?

@ramimoshe
Copy link
Contributor Author

@ramimoshe ramimoshe commented Sep 29, 2017

BTW I will try to add tests in my other open PR

@lpinca
Copy link
Member

@lpinca lpinca commented Sep 29, 2017

@ramimoshe that's how we land PRs you can find more info on the process in our documentation.
@Trott already opened a PR for the test. See #15678.

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
PR-URL: nodejs/node#15603
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants