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

docs(new): migrate HTTPResponse docs to TSDoc #6085

Merged
merged 2 commits into from Jun 25, 2020
Merged

Conversation

hanselfmu
Copy link
Collaborator

No description provided.

ok(): boolean {
// TODO: document === 0 case?
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mathiasbynens do you have any context to this case? I'm not sure why we'd have this.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know, but I would guess this is used for errors before we can even get an HTTP response + status code, e.g. certificate issues. Maybe @sigurdschneider knows?

Copy link
Collaborator

@jackfranklin jackfranklin left a comment

Choose a reason for hiding this comment

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

Minor comments, looks great on the whole, thank you!

Copy link
Collaborator

@jackfranklin jackfranklin left a comment

Choose a reason for hiding this comment

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

This looks good to me ! If we're not sure on the status code 0 case, happy to merge as is.

@jackfranklin
Copy link
Collaborator

I'm going to merge this, and we can make another PR to address the status = 0 case.

@jackfranklin jackfranklin merged commit c7851e8 into main Jun 25, 2020
@jackfranklin jackfranklin deleted the tsdoc-httpresponse branch June 25, 2020 09:26
mathiasbynens pushed a commit that referenced this pull request Jun 25, 2020
Co-authored-by: Changhao Han <changhaohan@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants