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.

Sign up for GitHub

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

Jump to bottom

url, test: add inspect to TupleOrigin #10039

Closed
wants to merge 4 commits into from
Closed

url, test: add inspect to TupleOrigin #10039

wants to merge 4 commits into from

Conversation

captainsafia
Copy link
Contributor

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

url, test

Description of change

This PR adds code and tests for an inspect function on the TupleOrigin object.

@captainsafia
url: add inspect function to TupleOrigin
c6b324e 
This adds a simple inspect function the the TupleOrigin class.
@captainsafia
test: add test for inspect in TupleOrigin
b930653 
This adds tests for the newly added inspect function in the TupleOrigin
class.
@nodejs-github-bot nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Dec 1, 2016
@captainsafia captainsafia changed the title url, test: add inspect to tuple origin url, test: add inspect to TupleOrigin Dec 1, 2016
@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 2, 2016
@Trott
Copy link
Member

Trott commented Dec 2, 2016

/cc @jasnell @addaleax

addaleax
addaleax approved these changes Dec 2, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM with a suggestion, thanks!

lib/internal/url.js
@@ -69,6 +69,15 @@ class TupleOrigin {
result += `:${this.port}`;
return result;
}

inspect() {
return `TupleOrigin{
Copy link
Member

Choose a reason for hiding this comment

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

I think we have spaces before the { in most cases here?

cjihrig
cjihrig approved these changes Dec 2, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with @addaleax's nit addressed and a CI run.

@jasnell
Copy link
Member

jasnell commented Dec 2, 2016 via email

@captainsafia
Copy link
Contributor Author

Nit addressed.

@gibfahn
Copy link
Member

gibfahn commented Dec 2, 2016

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

@addaleax
Copy link
Member

addaleax commented Dec 2, 2016

@captainsafia Looks like you need to update the test files too? :)

@addaleax
Copy link
Member

addaleax commented Dec 2, 2016

Thanks for being so quick to update! Here’s a new CI: https://ci.nodejs.org/job/node-test-commit/6348/

@captainsafia
Copy link
Contributor Author

@addaleax: Whoops. Excuse my merge-eager fingers. Fixed now. 😊

@addaleax
Copy link
Member

addaleax commented Dec 5, 2016

Landed in 8831732, thanks for the contribution!

@addaleax addaleax closed this Dec 5, 2016
addaleax pushed a commit that referenced this pull request Dec 5, 2016
@captainsafia @addaleax
url: add inspect function to TupleOrigin
8831732 
This adds a simple inspect function the the TupleOrigin class.
This adds tests for the newly added inspect function in the TupleOrigin
class.

PR-URL: #10039
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@jasnell
Copy link
Member

jasnell commented Dec 5, 2016

Thank you for this @captainsafia ... this one has been on my list of todo's for remaining items on the new URL implementation and I really appreciate the contribution!

@captainsafia captainsafia deleted the add-inspect-to-tuple-origin branch December 5, 2016 07:09
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
@captainsafia
url: add inspect function to TupleOrigin
a760575 
This adds a simple inspect function the the TupleOrigin class.
This adds tests for the newly added inspect function in the TupleOrigin
class.

PR-URL: nodejs#10039
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 15, 2016
@captainsafia @MylesBorins
url: add inspect function to TupleOrigin
24482d0 
This adds a simple inspect function the the TupleOrigin class.
This adds tests for the newly added inspect function in the TupleOrigin
class.

PR-URL: #10039
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
Merged
italoacasas added a commit that referenced this pull request Dec 15, 2016
@italoacasas
2016-12-20, Version 7.3.0 (Current)
617dbf7 
Notable changes

SEMVER-MINOR
- url:
    - add inspect function to TupleOrigin (Safia Abdalla) #10039
- crypto:
    - allow adding extra certs to well-known CAs (Sam Roberts) #9139

SEMVER-PATCH
- buffer:
    - fix single-character string filling (Anna Henningsen) #9837
    - handle UCS2 .fill() properly on BE (Anna Henningsen) #9837
- url:
    - including base argument in originFor (joyeecheung) #10021
    - improve URLSearchParams spec compliance (Timothy Gu) #9484
- http:
    - remove stale timeout listeners (Karl Böhlmark) #9440
- build:
    - fix node_g target (Daniel Bevenius) #10153
- fs:
    - remove unused argument from copyObject() (Ethan Arrowood) #10041
- timers:
    - fix handling of cleared immediates (hveldstra) #9759

PR-URL: #10277
italoacasas added a commit that referenced this pull request Dec 17, 2016
@italoacasas
2016-12-20, Version 7.3.0 (Current)
0afa836 
Notable changes:
* **crypto**:
  - Allow adding extra certificates to well-known CAs. (Sam Roberts)
    [#9139](#9139)
* **buffer**:
  - Fix single-character string filling. (Anna Henningsen)
    [#9837](#9837)
  - Handle UCS2 `.fill()` properly on BE. (Anna Henningsen)
    [#9837](#9837)
* **url**:
  - Add inspect function to TupleOrigin. (Safia Abdalla)
    [#10039](#10039)
  - Including base argument in originFor. (joyeecheung)
    [#10021](#10021)
  - Improve URLSearchParams spec compliance. (Timothy Gu)
    [#9484](#9484)
* **http**:
  - Remove stale timeout listeners. (Karl Böhlmark)
    [#9440](#9440)
* **build**:
  - Fix node_g target. (Daniel Bevenius)
    [#10153](#10153)
* **fs**:
  - Remove unused argument from copyObject(). (EthanArrowood)
    [#10041](#10041)
* **timers**:
  - Fix handling of cleared immediates. (hveldstra)
    [#9759](#9759)
* **src**:
  - Add wrapper for process.emitWarning(). (SamRoberts)
    [#9139](#9139)
  - Fix string format mistake for 32 bit node.(Alex Newman)
    [#10082](#10082)
@joyeecheung joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. semver-minor PRs that contain new features and should be released in the next minor version. url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@captainsafia @Trott @jasnell @gibfahn @addaleax @cjihrig @imyller @joyeecheung @nodejs-github-bot