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

Add CustomInspect for Headers #3130

Merged
merged 1 commit into from Oct 28, 2019

Conversation

@RoryMalcolm
Copy link
Contributor

RoryMalcolm commented Oct 15, 2019

Fixes: denoland/deno_std#543

cli/js/headers.ts

  • Adds an implementation of CustomInspect for the Headers class
  • Worth noting due to implementation of the Headers class the contents of headersMap have lowercase keys, although this matches the specification as header keys are case agnostic, it does seem to not match behaviour of other implementations in other languages I have seen, would require some rewriting of Headers.ts

cli/js/headers_test.ts

  • Adds a test to ensure that the output matches expectations
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Oct 15, 2019

CLA assistant check
All committers have signed the CLA.

]);
assertEquals(
stringify(multiParamHeader),
"Headers {content-type: Application/json, content-length: 1337}"

This comment has been minimized.

Copy link
@nayeemrmn

nayeemrmn Oct 15, 2019

Contributor

This should have spaces near the braces: Headers { content-type: ... for consistency with how objects are printed.

output = output + `${key}: ${value}` + (headerSize == 1 ? "" : ", ");
headerSize--;
});
return `Headers {${output}}`;

This comment has been minimized.

Copy link
@nayeemrmn

nayeemrmn Oct 15, 2019

Contributor

This should have spaces near the braces: Headers { content-type: ... for consistency with how objects are printed.

The implementation as well :) Done while writing.

@RoryMalcolm

This comment has been minimized.

Copy link
Contributor Author

RoryMalcolm commented Oct 15, 2019

prettier --l cli/js/headers_test.ts returns nothing 🤔

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Oct 15, 2019

./tools/format.py

@RoryMalcolm

This comment has been minimized.

Copy link
Contributor Author

RoryMalcolm commented Oct 15, 2019

./tools/format.py

CI says its failing on prettier headers_test.ts, but I run:

$ prettier --check cli/js/headers_test.ts
Checking formatting...
All matched files use Prettier code style!

And this has no output:

$ prettier --write cli/js/headers_test.ts

Thanks for this.

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Oct 15, 2019

Does tools/format.py not fix it? It calls prettier - potentially with different configuration than the default prettier.

@RoryMalcolm RoryMalcolm force-pushed the RoryMalcolm:add-custominspect-for-headers branch from 092cfcf to 509278f Oct 15, 2019
@RoryMalcolm

This comment has been minimized.

Copy link
Contributor Author

RoryMalcolm commented Oct 27, 2019

Bringing this one back up after looking it over at the weekend after some strange issues I ran into.

$ ./target/debug/deno run --reload -A cli/tests//019_media_types.ts                                                    
Compile file:///Users/rorymalcolm/Documents/projects/deno/cli/tests/019_media_types.ts
Download http://localhost:4545/cli/tests/subdir/mt_text_typescript.t1.ts
Download http://localhost:4545/cli/tests/subdir/mt_video_vdn.t2.ts
Download http://localhost:4545/cli/tests/subdir/mt_video_mp2t.t3.ts
Download http://localhost:4545/cli/tests/subdir/mt_application_x_typescript.t4.ts
Download http://localhost:4545/cli/tests/subdir/mt_text_javascript.j1.js
Download http://localhost:4545/cli/tests/subdir/mt_application_ecmascript.j2.js
Download http://localhost:4545/cli/tests/subdir/mt_text_ecmascript.j3.js
Download http://localhost:4545/cli/tests/subdir/mt_application_x_javascript.j4.js
error: Uncaught HttpOther: http://localhost:4545/cli/tests/subdir/mt_text_typescript.t1.ts: error trying to connect: Connection refused (os error 61)
► $deno$/dispatch_json.ts:40:11
    at DenoError ($deno$/errors.ts:20:5)
    at unwrapResponse ($deno$/dispatch_json.ts:40:11)
    at sendAsync ($deno$/dispatch_json.ts:91:10)

I get this error when running the failing media types test, which looks like a permissions issue? Or the webserver is shutting down before the test completes after running into a failure? I think its beyond my knowledge of the deno testing codebase at the moment.

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Oct 27, 2019

I think it’s just a flaky test. Try rerunning CI by committing an empty commit: git commit —allow-empty -m bump && git push

cli/js/headers.ts
- Adds an implementation of CustomInspect for the Headers class
- Worth noting due to implementation of the Headers class the contents of headersMap have lowercase keys, although this matches the specification as header keys are case agnostic it does seem to not match behaviour of other implementations in other languages I have seen, would require some rewriting of Headers.ts

cli/js/headers_test.ts
- Adds a test to ensure that the output matches expectations
@RoryMalcolm RoryMalcolm force-pushed the RoryMalcolm:add-custominspect-for-headers branch from 6497953 to c3f5523 Oct 27, 2019
@ry
ry approved these changes Oct 28, 2019
Copy link
Collaborator

ry left a comment

LGTM - thanks @RoryMalcolm !

@ry ry merged commit 967c236 into denoland:master Oct 28, 2019
10 checks passed
10 checks passed
test macOS-10.14
Details
test_std macOS-10.14
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.