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

Missing documentation for tls.parseCertString #14193

Closed
chrisbroome opened this issue Jul 12, 2017 · 10 comments
Closed

Missing documentation for tls.parseCertString #14193

chrisbroome opened this issue Jul 12, 2017 · 10 comments
Labels
doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem.

Comments

@chrisbroome
Copy link

  • Version: all as of v8.1.4
  • Platform: all
  • Subsystem: all

There is currently no documentation for tls.parseCertString

Since this function exported, I'd consider it part of the public API and therefore it should probably have some documentation explaining what it does.

@mscdex mscdex added doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem. labels Jul 12, 2017
@bnoordhuis
Copy link
Member

That function was exposed by accident more than anything else in commit af80e7b from 2013. It's probably better to move it to lib/internal than promote it to official API.

cc @indutny

@chrisbroome
Copy link
Author

chrisbroome commented Jul 12, 2017

@bnoordhuis Makes sense to me.

Also, after looking it what it actually does, it's also almost identical to calling

querystring.parse(str, '\n', '=')

with the one exception being when the string is invalid:

querystring.parse('invalidCertString', '\n', '=')
// yields {invalidCertString: ''} instead of {}

@XadillaX
Copy link
Contributor

If we move it to internal, I think it's a semver major.

@sam-github
Copy link
Contributor

@chrisbroome did you call this API? for what purpose? It parses a format that doesn't appear to ever be produced by a public API, I don't see how it could be useful.

Calling it semver-major is safe, lets not doc, and move to internal for 9.x

@chrisbroome
Copy link
Author

I never actually used the API in production code. Basically I was playing around in the REPL exploring what was available in the tls module. I just typed tls.<tab>, saw parseCertString, and wanted to know what its purpose was (I thought maybe it was a function that let you extract information from base64-encoded .pem files). Anyway, I went to the documentation, but the function wasn't there, so I decided to open the issue.

@sam-github
Copy link
Contributor

OK. I don't personally think we should document something only to remove it in the next major. If we do document it, it should be doc-deprecated at least.

@XadillaX
Copy link
Contributor

@sam-github I agree to remove it in the next major and document it now for deprecated.

XadillaX added a commit to XadillaX/node that referenced this issue Jul 13, 2017
@bnoordhuis
Copy link
Member

I'd do the following:

  1. Don't document it.
  2. Move it to lib/internal and leave a stub function in lib/tls.js that prints a deprecation warning.
  3. In the next major or the one after that, remove the stub.

XadillaX added a commit to XadillaX/node that referenced this issue Jul 13, 2017
`tls.parseCertString()` exposed by accident. Now move this function to
`internal/tls` and mark the original one as deprecated.

Refs: nodejs#14193
Refs: nodejs@af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188
@XadillaX
Copy link
Contributor

@bnoordhuis I've created another PR #14218 to move the function to internal.

XadillaX added a commit to XadillaX/node that referenced this issue Jul 23, 2017
`tls.parseCertString()` was made public by mistack. So mark it as
deprecated.

Refs: nodejs#14193
Refs: nodejs@af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188
XadillaX added a commit to XadillaX/node that referenced this issue Aug 4, 2017
`tls.parseCertString()` exposed by accident. Now move this function to
`internal/tls` and mark the original one as deprecated.

Refs: nodejs#14193
Refs: nodejs@af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188
XadillaX added a commit to XadillaX/node that referenced this issue Aug 10, 2017
`tls.parseCertString()` was made public by mistack. So mark it as
deprecated.

Refs: nodejs#14193
Refs: nodejs@af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188
BridgeAR pushed a commit that referenced this issue Sep 8, 2017
`tls.parseCertString()` was made public by mistack. So mark it as
deprecated.

PR-URL: #14245
Refs: #14193
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
XadillaX added a commit to XadillaX/node that referenced this issue Sep 8, 2017
`tls.parseCertString()` exposed by accident. Now move this function to
`internal/tls` and mark the original one as deprecated.

Refs: nodejs#14193
Refs: nodejs@af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188
addaleax pushed a commit to addaleax/node that referenced this issue Sep 13, 2017
`tls.parseCertString()` was made public by mistack. So mark it as
deprecated.

PR-URL: nodejs#14245
Refs: nodejs#14193
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BridgeAR pushed a commit that referenced this issue Sep 13, 2017
`tls.parseCertString()` exposed by accident. Now move this function to
`internal/tls` and mark the original one as deprecated.

PR-URL: #14249
Refs: #14193
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
addaleax pushed a commit to addaleax/ayo that referenced this issue Sep 17, 2017
`tls.parseCertString()` exposed by accident. Now move this function to
`internal/tls` and mark the original one as deprecated.

PR-URL: nodejs/node#14249
Refs: nodejs/node#14193
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
jasnell pushed a commit that referenced this issue Sep 20, 2017
`tls.parseCertString()` was made public by mistack. So mark it as
deprecated.

PR-URL: #14245
Refs: #14193
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this issue Sep 21, 2017
`tls.parseCertString()` exposed by accident. Now move this function to
`internal/tls` and mark the original one as deprecated.

PR-URL: nodejs/node#14249
Refs: nodejs/node#14193
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@bnoordhuis
Copy link
Member

Closing, #14249 was merged last year.

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. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants