FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util: add guessHandleType method by evanlucas · Pull Request #2797 · nodejs/node · GitHub

/ node Public

util: add guessHandleType method - #2797

Closed
evanlucas wants to merge 1 commit into
nodejs:masterfrom
evanlucas:guesshandle
ConversationCommits (1)ChecksFiles changed
Closed

util: add guessHandleType method#2797
evanlucas wants to merge 1 commit into
nodejs:masterfrom
evanlucas:guesshandle

Conversation

evanlucas commented Sep 10, 2015

Copy link
Copy Markdown
Contributor

With moving in the direction of deprecating process.binding,
it makes sense to expose the ability to guess the handle type
of an fd.

Does it make sense that this is exposed in tty?

An example of where this would be useful is debug which
is a quite popular package. (https://github.com/visionmedia/debug/blob/master/node.js#L144-L203)

Related: #2768

evanlucas added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 10, 2015

evanlucas commented Sep 10, 2015

Copy link
Copy Markdown
Contributor Author

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

mscdex commented Sep 10, 2015

Copy link
Copy Markdown
Contributor

+1 to making it public, but I don't think tty is the best place to put it. Perhaps util would be better?

evanlucas commented Sep 10, 2015

Copy link
Copy Markdown
Contributor Author

I don't want to bloat util, but I think it makes more sense.

util: add guessHandleType method
0edc7a5 
With moving in the direction of deprecating process.binding,
it makes sense to expose the ability to guess the handle type
of an fd.
evanlucas changed the title tty: add guessHandleType method util: add guessHandleType method Sep 10, 2015

evanlucas commented Sep 10, 2015

Copy link
Copy Markdown
Contributor Author

Ok, changed to util

bnoordhuis commented Sep 10, 2015

Copy link
Copy Markdown
Member

Has anyone actually asked for this? I know the debug module uses tty_wrap.guessHandleType() but that's because it copied createWritableStdioStream() from src/node.js wholesale.

evanlucas commented Sep 10, 2015

Copy link
Copy Markdown
Contributor Author

Not to my knowledge. Just wanted to take some proactive steps to when we actually deprecate process.binding

bnoordhuis commented Sep 10, 2015

Copy link
Copy Markdown
Member

Okay, in that case I'm -1 until a reasonable use case pops up. Adding an API is easy but if it turns out to be the wrong API, we're stuck with it. Proactivity is what got us the util.is*() functions.

evanlucas commented Sep 10, 2015

Copy link
Copy Markdown
Contributor Author

Fair enough

evanlucas closed this Sep 10, 2015
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL