ffi: remove function signature property aliases - #63482
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
Signed-off-by: Renegade334 <contact.9a5d6388@renegade334.me.uk>
Codecov Report❌ Patch coverage is 90.00000% with 6 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #63482 +/- ##
==========================================
- Coverage 90.17% 90.13% -0.05%
==========================================
Files 718 718
Lines 227731 228342 +611
Branches 42768 42919 +151
==========================================
+ Hits 205365 205807 +442
- Misses 14145 14274 +129
- Partials 8221 8261 +40
... and 64 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
| * `result`, `return`, or `returns` for the return type. | ||
| * `parameters` or `arguments` for the parameter type list. | ||
| * `result` {string} A [type name][type names] specifying the return type of the | ||
| function or callback. **Default:** `'void'`. |
There was a problem hiding this comment.
If I can cast a vote for keeping return instead of result, I'd like to do so – that's just generally more in line with what the standardized terminology around function signatures is
Sorry, something went wrong.
There was a problem hiding this comment.
Well, if I have to pick one I'd go for return and arguments.
@Renegade334 Are you fine with doing so?
Sorry, something went wrong.
There was a problem hiding this comment.
Fine by me.
Sorry, something went wrong.
There was a problem hiding this comment.
Ok, if @addaleax concurs I think you can continue with the changes.
Sorry, something went wrong.
| ERR_INTERNAL_ASSERTION, | ||
| }, | ||
| } = require('internal/errors'); | ||
| const assert = require('internal/assert'); |
There was a problem hiding this comment.
A lot of the assertions had to be modified anyway with this changeset, so as a driveby I've changed them all to the canonical internal assertion pattern.
Sorry, something went wrong.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Renegade334 <contact.9a5d6388@renegade334.me.uk> PR-URL: #63482 Refs: https://github.com/nodejs/node/pull/62072/changes#r3067834658 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Follow-up to df09b2a. Refs: nodejs#63482 Signed-off-by: Anna Henningsen <anna@addaleax.net>
Refs: https://github.com/nodejs/node/pull/62072/changes#r3067834658
Having multiple property name aliases for function signature objects doesn't add utility, but from a TS perspective, it blocks being able to infer the resultant function type from the shape of the signature object. With all else being equal, it'd be useful to get rid of them.