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

src: rename CONNECTION provider to SSLCONNECTION #12989

Closed

Conversation

@danbev
Copy link
Member

commented May 12, 2017

Currently the async provider type CONNECTION is used in node_crypto.h
and it might be clearer if it was named SSLCONNECTION as suggested by
addaleax.

This commit renames only the provider type as I was not sure if it was
alright to change the class Connection as well.

Refs: #12967 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

src: rename CONNECTION provider to SSLCONNECTION
Currently the async provider type CONNECTION is used in node_crypto.h
and it might be clearer if it was named SSLCONNECTION as suggested by
addaleax.

This commit renames only the provider type as I was not sure if it was
alright to change the class Connection as well.

Refs: #12967 (comment)
@danbev

This comment has been minimized.

Copy link
Member Author

commented May 12, 2017

@cjihrig
Copy link
Contributor

left a comment

LGTM. cc: @addaleax who I think requested this.

@refack
refack approved these changes May 12, 2017
@addaleax
Copy link
Member

left a comment

This commit renames only the provider type as I was not sure if it was
alright to change the class Connection as well.

You can, if you want, but there it’s clear from context what the class is about (I think).

@refack

This comment has been minimized.

Copy link
Member

commented May 12, 2017

This commit renames only the provider type as I was not sure if it was alright to change the class Connection as well.

You can, if you want, but there it’s clear from context what the class is about (I think).

IMHO that it's in namespace crypto is good.

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 12, 2017

@addaleax @refack Good point, lets leave it as is. Thanks

danbev added a commit to danbev/node that referenced this pull request May 15, 2017
src: rename CONNECTION provider to SSLCONNECTION
Currently the async provider type CONNECTION is used in node_crypto.h
and it might be clearer if it was named SSLCONNECTION as suggested by
addaleax.

This commit renames only the provider type as I was not sure if it was
alright to change the class Connection as well.

Refs: nodejs#12967 (comment)
PR-URL: nodejs#12989
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@danbev

This comment has been minimized.

Copy link
Member Author

commented May 15, 2017

Landed in 60f0dc7

@danbev danbev closed this May 15, 2017

@danbev danbev deleted the danbev:rename-async-connection-provider branch May 15, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
src: rename CONNECTION provider to SSLCONNECTION
Currently the async provider type CONNECTION is used in node_crypto.h
and it might be clearer if it was named SSLCONNECTION as suggested by
addaleax.

This commit renames only the provider type as I was not sure if it was
alright to change the class Connection as well.

Refs: nodejs#12967 (comment)
PR-URL: nodejs#12989
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jun 22, 2017

should this be landed on v6.x? Assuming it would be part of a larger backport of async_hooks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.