os: expose guessFileDescriptorType#58060
Conversation
nodejs-github-bot
added
c++
|
I'll rebase to fix the first commit message after the CI passes! Should I also squash the PR or leave the rest as is? |
As you prefer, in any case it will get squashed upon merging. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #58060 +/- ##
==========================================
- Coverage 89.81% 89.77% -0.04%
==========================================
Files 697 695 -2
Lines 215773 214702 -1071
Branches 41279 41147 -132
==========================================
- Hits 193786 192744 -1042
- Misses 14093 14154 +61
+ Partials 7894 7804 -90
🚀 New features to boost your workflow:
|
vladfrangu
force-pushed
the
feat/expose-guessHandleType-on-os
branch
from
b88d406 to
f3d0b71
Compare
doc/api/os.md
Outdated
| added: REPLACEME | ||
| --> | ||
|
|
||
| * `handle` {integer} The handle number to try and guess the type of. |
There was a problem hiding this comment.
This should really specify that "handle" means "file descriptor". In fact, given that the fs docs almost exclusively use "file descriptor" and "fd", I would also adopt that terminology here.
There was a problem hiding this comment.
Then I could also rename the function to guessFileDescriptorType? Or should I leave it as is?
And will apply the changes to fd/file descriptor in a few
There was a problem hiding this comment.
Yeah, I'd probably do that. In the context of libuv, guessHandleType makes more sense, because it returns a value that actually corresponds to the types of libuv handles, which wrap around fds.
There was a problem hiding this comment.
Done! Let me know if I've also done the rebase correctly 🤞
vladfrangu
changed the title
guessHandleTypeguessFileDescriptorType
vladfrangu
force-pushed
the
feat/expose-guessHandleType-on-os
branch
from
f3d0b71 to
0c33ea7
Compare
vladfrangu
force-pushed
the
feat/expose-guessHandleType-on-os
branch
from
0c33ea7 to
93dc673
Compare
src/node_util.cc
Outdated
| proxy->GetTarget(), | ||
| proxy->GetHandler() | ||
| }; | ||
| Local<Value> ret[] = {proxy->GetTarget(), proxy->GetHandler()}; |
There was a problem hiding this comment.
There are a large number of unrelated changes in here. Were these manually edited or did the format tool do these?
There was a problem hiding this comment.
All of the formatting changes were done by saving the file (after configuring vscode with the settings example provided by the contributing guide). Not sure exactly why so many changes happened, but running the cpp formatter through make also didn't revert it. I can manually revert it if desired
There was a problem hiding this comment.
Please do, making PRs easier to review gives them the best chance to actually land, and it avoids polluting git blame
There was a problem hiding this comment.
So, I did revert these changes, and staged them, but after running make format-cpp, the file was reformatted to how it is on this PR. I'll push the non-formatted change but not sure if I should open a PR after formatting the file or just leave it be
There was a problem hiding this comment.
Yet running CLANG_FORMAT_START=main make format-cpp changes nothing, confusing.. I'll leave that be for now
vladfrangu
force-pushed
the
feat/expose-guessHandleType-on-os
branch
from
d803730 to
15f0bda
Compare
vladfrangu
force-pushed
the
feat/expose-guessHandleType-on-os
branch
from
15f0bda to
a65ced5
Compare
| * Returns: {string|null} | ||
|
|
||
| Returns the type of the file descriptor passed in, or `null` if the provided file descriptor | ||
| is invalid. |
There was a problem hiding this comment.
Nit: some expanded explanation about why this is useful would probably be helpful. It's rather obscure.
There was a problem hiding this comment.
I tried to improve the documentation, but I'm not sure if this is good 😅
| // For an unhandled handle type, we want to return `UNKNOWN` instead of | ||
| // `null` since the type is "known" by UV, just not exposed further to | ||
| // JS land | ||
| return 5; |
There was a problem hiding this comment.
Nit... for later... not something to do in this PR... these really ought to be defined in an enum
There was a problem hiding this comment.
I would do it if I knew how 😅, and there is a TODO for that already in the function
Exposes the internal `guessHandleType` function as `guessFileDescriptorType`, which can be used to see if a handle has a specific type, regardless of the OS it is on. This helps out with detecting, for example, if standard input is piped into the process, instead of relying on file system calls. Refs: nodejs#57603
Co-authored-by: James M Snell <jasnell@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Tested with `require('internal/test/binding').internalBinding('util').guessHandleType(2**31)` (not sure if there was a better way, but it works so)
Signed-off-by: Vlad Frangu <me@vladfrangu.dev>
vladfrangu
force-pushed
the
feat/expose-guessHandleType-on-os
branch
from
52dd182 to
520693d
Compare
|
I've rebased the PR, let me know if I should also squash it (for the signed off message) or such 🙏 |
aduh95
added
author ready
|
Also let me know if there are other changes I should do (improvements to docs, etc) The macOS ci failure is confusing, locally all tests passed so I'm guessing its flakiness? |
github-actions
bot
removed
the
request-ci
7 participants
Exposes the internal
guessHandleTypefunction, which can be used to see if a handle has a specific type, regardless of the OS it is on.This helps out with detecting, for example, if standard input is piped into the process, instead of relying on file system calls.
Refs: #57603