-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Port some commits from from joyent/node@master #1770
Conversation
Mehhhh, that's so un-JavaScriptey. The repl commit is the better approach. We could make the linter disallow using Buffer as a global. |
👍 Not exactly sure how though. :P Maybe cc @silverwind? |
You could disable the Buffer global (and all other predefined ones) by removing this here: https://github.com/nodejs/io.js/blob/9da168b71fb729635ad71e839630480e815623d0/.eslintrc#L2 Afterwards, specify all globals manually here: https://github.com/nodejs/io.js/blob/9da168b71fb729635ad71e839630480e815623d0/.eslintrc#L78 If you want something readonly add it as |
I agree with the general thrust of 7532d24 but it isn't comprehensive enough, it only updates some of the files in lib/.
|
@silverwind that didn't seem to work for Buffer. Also turning off |
@Fishrock123 I think that's stuff for another PR. When disabling |
9dbc947
to
03f0720
Compare
Ok, removed that commit since it's not comprehensive and we don't have a good way to keep linting it yet. |
@silverwind mind signing off on the remaining two commits here? |
LGTM On a sidenote, I noticed |
Weird. Noted. |
- `sh.css` already exists in `api_assets` - `sh_vim-dark.css` is unused, but used in the repo `node-website` now Reviewed-by: Trevor Norris <trev.norris@gmail.com> Signed-off-by: Julien Gilli <julien.gilli@joyent.com> PORT-FROM: joyent/node @ 0c50195 PR-URL: nodejs#1770 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Clarify that synchronous functions in fs with no return value return undefined. Specify that fs.openSync() returns an integer and fs.existsSync() returns true or false. Fixes: nodejs/node-v0.x-archive#9313 PR: nodejs/node-v0.x-archive#9359 Reviewed-By: Julien Gilli <julien.gilli@joyent.com> PORT-FROM: joyent/node @ 51fe319 PR-URL: nodejs#1770 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Conflicts: doc/api/fs.markdown
03f0720
to
a79dece
Compare
- `sh.css` already exists in `api_assets` - `sh_vim-dark.css` is unused, but used in the repo `node-website` now Reviewed-by: Trevor Norris <trev.norris@gmail.com> Signed-off-by: Julien Gilli <julien.gilli@joyent.com> PORT-FROM: joyent/node @ 0c50195071a57bc40df82c8f57d341a435ff1eb6 PR-URL: nodejs/node#1770 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Clarify that synchronous functions in fs with no return value return undefined. Specify that fs.openSync() returns an integer and fs.existsSync() returns true or false. Fixes: nodejs/node-v0.x-archive#9313 PR: nodejs/node-v0.x-archive#9359 Reviewed-By: Julien Gilli <julien.gilli@joyent.com> PORT-FROM: joyent/node @ 51fe319faf4399fd027f8b32d1c425200b911e44 PR-URL: nodejs/node#1770 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Conflicts: doc/api/fs.markdown
Ports a couple of commits over from joyent/node (master) that would be useful here.
Refs: #17, #21, #31
7532d24 repl: Private Buffer object in lib/* files
should probably have a test of some sort, or we should somehow check that removing the Buffer global doesn't cause issues in core somewhere else.Chris suggested running
Buffer = {}
though a repl instance, but I'm pretty sure that would only catch the readline module without extensive use of other modules.Generally, I'm still leaning towards making
global.Buffer
non-writable.