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

Adds custom inspect method for URL #3241

Merged
merged 2 commits into from Oct 31, 2019

Conversation

@svnv
Copy link
Contributor

svnv commented Oct 29, 2019

Adds a custom inspection function for the URL class so that some hardcoded properties will always be listed in full.

Eg.

> new URL("http://example.com/?");
URL { href: "http://example.com/?", origin: "http://example.com", protocol: "http:", username: "", password: "", host: "example.com", hostname: "example.com", port: "", pathname: "/", hash: "", search: "?" }

Closes #3124

Using custom inspection function implemented in #2791

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Oct 29, 2019

CLA assistant check
All committers have signed the CLA.

@svnv

This comment has been minimized.

Copy link
Contributor Author

svnv commented Oct 30, 2019

Another and slightly more generic approach here would be to add something like this

[customInspectProperties]:["href", "origin" ....]

to the URL class and handle the presentation of those in the console.ts file instead.

@ry
ry approved these changes Oct 30, 2019
cli/js/url_test.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

ry left a comment

@kitsonk please review if you have a chance.

Copy link
Contributor

kitsonk left a comment

One thought about the test, but otherwise looks good.

cli/js/url_test.ts Outdated Show resolved Hide resolved
"hash",
"search"
];
const objectString = keys

This comment has been minimized.

Copy link
@kitsonk

kitsonk Oct 30, 2019

Contributor

Fine for now, but longer term we should think about exporting helper function from console.ts to do consistent formatting and coloring of the output, but that is bigger scope than this PR.

@svnv

This comment has been minimized.

Copy link
Contributor Author

svnv commented Oct 31, 2019

Updated pull request after code review.

@ry
ry approved these changes Oct 31, 2019
Copy link
Collaborator

ry left a comment

LGTM - thanks!

@ry ry merged commit d7a5aed into denoland:master Oct 31, 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.