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

util: moving internal helpers to internal module by thefourtheye · Pull Request #2025 · nodejs/node · GitHub

/ node Public

util: moving internal helpers to internal module - #2025

Closed
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:util-refactoring
ConversationCommits (1)ChecksFiles changed
Closed

util: moving internal helpers to internal module#2025
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:util-refactoring

Conversation

thefourtheye commented Jun 20, 2015

Copy link
Copy Markdown
Contributor
  1. Moving _errnoException and _exceptionWithHostPort to
    internal/util module as they are internal helper functions. They
    should not be exposed as part of the util module.
  2. Issuing a deprecation warning when those functions are used.

mscdex added the util Issues and PRs related to the built-in util module. label Jun 20, 2015
cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 20, 2015
evanlucas reviewed Jun 20, 2015
View reviewed changes
Comment thread lib/fs.js

evanlucas Jun 20, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Wouldn't this break graceful-fs again?

vkurchatkin Jun 20, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It would. It's probably better to add a test for this to prevent accidental breakage

vkurchatkin commented Jun 20, 2015

Copy link
Copy Markdown
Contributor

@cjihrig why semver-major?

brendanashworth reviewed Jun 21, 2015
View reviewed changes
Comment thread lib/internal/util.js

brendanashworth Jun 21, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since this is going into an internal module, it doesn't need the underscore name.

Also, can we name the function (exports.errnoException = function errnoException(..) {) so we don't have to deal with exports.errnoException() below?

brendanashworth commented Jun 21, 2015

Copy link
Copy Markdown
Contributor

I'm going to relabel this as semver-minor as backwards compatibility is kept.

brendanashworth added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 21, 2015
util: moving internal helpers to internal module
70b7d9f 
1. Moving `_errnoException` and `_exceptionWithHostPort` to
   `internal/util` module as they are internal helper functions. They
   should not be exposed as part of the `util` module.

2. Issuing a deprecation warning when those functions are used.
Fishrock123 reviewed Jun 21, 2015
View reviewed changes
Comment thread lib/util.js

Fishrock123 Jun 21, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Isn't this useful for making errors from libuv bindings in userland?

ChALkeR commented Jun 21, 2015

Copy link
Copy Markdown
Member

There are other modules this might break. But it's probably their fault.
For example, karma-assert sets util._errnoException for some reason:

k/karma-assert-1.0.0.tgz/assert.js:815:util._errnoException = function(err, syscall, original) {

thefourtheye mentioned this pull request Jun 21, 2015
Closed

thefourtheye commented Jun 21, 2015

Copy link
Copy Markdown
Contributor Author

Closing this as it breaks graceful-fs and userland functions might make errors from libuv bindings with _errnoException helper.

thefourtheye deleted the util-refactoring branch June 21, 2015 03:46
thefourtheye mentioned this pull request Sep 6, 2015
Closed
ChALkeR mentioned this pull request Feb 5, 2016
Closed

ChALkeR commented Aug 17, 2016

Copy link
Copy Markdown
Member

@thefourtheye Perhaps it's time to reopen and revisit, as #6413 is going to be landed soon?

We would probably need a deprecation message on util._errnoException, though.

This was referenced Aug 17, 2016
Closed
Closed
ChALkeR mentioned this pull request Aug 26, 2016
Closed
2 tasks

ChALkeR commented Aug 29, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

@thefourtheye, #6413 landed, nothing is blocking this anymore. Do you wish to redo/reopen this PR?

gr00nd mentioned this pull request Feb 6, 2024
Open
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. util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL