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

readline: remove deprecated methods #6423

Merged
merged 1 commit into from
May 2, 2016
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 27, 2016

Checklist
  • tests and code linting passes (except currently broken tick processor)
  • the commit message follows commit guidelines
Affected core subsystem(s)

readline

Description of change

This commit removes the deprecated exports getStringWidth(), stripVTControlCharacters(), and isFullWidthCodePoint(). It also removes codePointAt() in its entirety, as it was deprecated and
not being used by core.

@cjihrig cjihrig added readline Issues and PRs related to the built-in readline module. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 27, 2016
@addaleax
Copy link
Member

LGTM. For anybody who wants to look it up, these were deprecated in #3862.

@bnoordhuis
Copy link
Member

LGTM. I have used isFullWidthCodePoint() in the past but it's not hard to break out into a module.

@mscdex
Copy link
Contributor

mscdex commented Apr 27, 2016

LGTM

@sindresorhus
Copy link

I have used isFullWidthCodePoint() in the past but it's not hard to break out into a module.

https://github.com/sindresorhus/is-fullwidth-code-point

@ChALkeR
Copy link
Member

ChALkeR commented Apr 27, 2016

LGTM is CI passes.
Previous greps at #3862 (comment), readline.codePointAt() is unused except for test-readline-interface.js copies here and there.

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

No comments after review.
LGTM

@thefourtheye
Copy link
Contributor

LGTM, if CI is green.

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

@nodejs/ctc ... any objections on this assuming CITGM is good?

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 30, 2016

@jasnell what is your take on the CITGM results?

@jasnell
Copy link
Member

jasnell commented Apr 30, 2016

The body-parser and glob failures are known and unrelated.
ws failed on debian (https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker/229/nodes=debian8-64/tapTestReport/smoker%2Ftest.tap-29/) which should be looked because it's output is not clear. It could just be an unrelated flaky failure.

@MylesBorins
Copy link
Contributor

Failure from ws

error:                     | 1) WebSocketServer #ctor emits an error if http   
error:                     | server bind fails:                                
error:                     | Uncaught Error: listen EADDRINUSE 0.0.0.0:50003   
error:                     | at Object.exports._errnoException (util.js:896:11)
error:                     | at exports._exceptionWithHostPort (util.js:919:20)
error:                     | at Server._listen2 (net.js:1253:14)               
error:                     | at listen (net.js:1289:10)                        
error:                     | at net.js:1399:9                                  
error:                     | at _combinedTickCallback                          
error:                     | (internal/process/next_tick.js:77:11)             
error:                     | at process._tickCallback                          
error:                     | (internal/process/next_tick.js:98:9)              
error:                     |                                                   
error:                     | 2) WebSocketServer #ctor will not crash when it   
error:                     | receives an unhandled opcode:                     
error:                     | Error: done() called multiple times               
error:                     | at Suite.<anonymous>                              
error:                     | (test/WebSocketServer.test.js:188:5)              
error:                     | at Suite.<anonymous>                              
error:                     | (test/WebSocketServer.test.js:28:3)               
error:                     | at Object.<anonymous>                             
error:                     | (test/WebSocketServer.test.js:27:1)               
error:                     | at require (internal/module.js:20:19)             
error:                     | at Array.forEach (native)                         
error:                     | at node.js:444:3                                  
error:                     |                                                   
error:                     | 3) WebSocketServer #ctor will not crash when it   
error:                     | receives an unhandled opcode:                     
error:                     | Error: done() called multiple times               
error:                     | at Suite.<anonymous>                              
error:                     | (test/WebSocketServer.test.js:188:5)              
error:                     | at Suite.<anonymous>                              
error:                     | (test/WebSocketServer.test.js:28:3)               
error:                     | at Object.<anonymous>                             
error:                     | (test/WebSocketServer.test.js:27:1)               
error:                     | at require (internal/module.js:20:19)             
error:                     | at Array.forEach (native)                         
error:                     | at node.js:444:3                                  
error:                     |                                                   
error:                     | 4) WebSocketServer #ctor will not crash when it   
error:                     | receives an unhandled opcode:                     
error:                     | Error: done() called multiple times               
error:                     | at Suite.<anonymous>                              
error:                     | (test/WebSocketServer.test.js:188:5)              
error:                     | at Suite.<anonymous>                              
error:                     | (test/WebSocketServer.test.js:28:3)               
error:                     | at Object.<anonymous>                             
error:                     | (test/WebSocketServer.test.js:27:1)               
error:                     | at require (internal/module.js:20:19)             
error:                     | at Array.forEach (native)                         
error:                     | at node.js:444:3                                  
error:                     |                                                   
error:                     | 5) WebSocketServer client properties upgradeReq is
error:                     | the original request object:                      
error:                     | Uncaught Error: socket hang up                    
error:                     | at createHangUpError (_http_client.js:250:15)     
error:                     | at Socket.socketCloseListener                     
error:                     | (_http_client.js:282:23)                          
error:                     | at TCP._handle.close [as _onclose] (net.js:492:12)
error:                     |                                                   
error:                     |                                                   
error:                     |                                                   
error:                     | Makefile:5: recipe for target 'run-tests' failed  
error:                     | make[1]: Leaving directory                        
error:                     | '/tmp/00837f4f-d71f-46d0-9e54-4b1241f98914/ws'    
error:                     | Mak… efile:27: recipe for target 'test' failed    

Looks like it could be flaky...

I've just released a new version of CITGM which should make the results much cleaner... not 100% why the tap results are truncated there... might be something wrong with the reporter, will dig into that

https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker/231/

This commit removes the deprecated exports getStringWidth(),
stripVTControlCharacters(), and isFullWidthCodePoint(). It also
removes codePointAt() in its entirety, as it was deprecated and
not being used by core.

Refs: nodejs#3862
PR-URL: nodejs#6423
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@cjihrig cjihrig merged commit 8a87b29 into nodejs:master May 2, 2016
@cjihrig cjihrig deleted the readline branch May 2, 2016 14:16
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request May 4, 2016
This commit removes the deprecated exports getStringWidth(),
stripVTControlCharacters(), and isFullWidthCodePoint(). It also
removes codePointAt() in its entirety, as it was deprecated and
not being used by core.

Refs: nodejs#3862
PR-URL: nodejs#6423
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants