Skip to content

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 27, 2016

Checklist
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc (querystring)

Description of change

General improvements to querystring.md copy

@nodejs/documentation @mscdex

@jasnell jasnell added doc Issues and PRs related to the documentations. querystring Issues and PRs related to the built-in querystring module. labels May 27, 2016
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if anyone ever actually uses these sep and eq parameters :)

@benjamingr
Copy link
Member

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

s/key+value/key and value/

@jasnell
Copy link
Member Author

jasnell commented May 27, 2016

Nits addressed

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I'm not sure the optional indicators here accurately reflect how the actual implementation does argument checking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was looking at that also. I need to go back in and verify this.

@jasnell
Copy link
Member Author

jasnell commented May 27, 2016

Updated

@jasnell
Copy link
Member Author

jasnell commented Jun 2, 2016

@mscdex ... ping ... I'm still going to go back and check the args against the actual args checking but I'd like to get this PR landed and if there are changes necessary I'll do those separately. Does this LGTY?

@nodejs/documentation

@mscdex
Copy link
Contributor

mscdex commented Jun 3, 2016

@jasnell except for the args issues, LGTM

@jasnell jasnell force-pushed the doc-querystring-copy branch from 0e7fcb4 to b9e5e68 Compare June 6, 2016 15:33
@jasnell
Copy link
Member Author

jasnell commented Jun 6, 2016

@mscdex ... I corrected the arg issues. Getting this landed!

jasnell added a commit that referenced this pull request Jun 6, 2016
PR-URL: #7023
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Brian White <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Jun 6, 2016

Landed in 80f1fbb

@jasnell jasnell closed this Jun 6, 2016
evanlucas pushed a commit that referenced this pull request Jun 15, 2016
PR-URL: #7023
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Brian White <[email protected]>
@evanlucas evanlucas mentioned this pull request Jun 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Issues and PRs related to the documentations. querystring Issues and PRs related to the built-in querystring module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants