Editorial: refactor common behaviour of SetFunctionName #1384
Conversation
| 1. Assert: Type(_name_) is either Symbol or String. | ||
| 1. Assert: If _prefix_ is present, then Type(_prefix_) is String. | ||
| 1. If ! HasOwnProperty(_F_, `"name"`) is *true*, return. |
There was a problem hiding this comment.
I’m not sure it’s as clear to have an operation named “set function name” not always set the function name, although i see how this DRYs up the steps. What do you think?
There was a problem hiding this comment.
You could change the name to something like EnsureFunctionName (i.e., it ensures that the function has a name, setting it only if necessary).
There was a problem hiding this comment.
I'm wondering the same thing as @ljharb. I like the general idea of cleaning things up with refactoring, but for specs we should prioritize clarity.
| 1. Assert: Type(_name_) is either Symbol or String. | ||
| 1. Assert: If _prefix_ is present, then Type(_prefix_) is String. | ||
| 1. If ! HasOwnProperty(_F_, `"name"`) is *true*, return. |
There was a problem hiding this comment.
Where the replaced code has a call to HasOwnProperty, that call is always preceded by '?'. I don't think you're justified in changing that to '!' here. Moreover, in those cases, the resulting call to SetFunctionName should be preceded by '?'.
devsnek
force-pushed
the
refactor/setfunctionname
branch
from
cca351f to
cda05c6
Compare
|
There exists usage of the SetFunctionName abstract operation in other non-algorithmic parts of the spec, such as in the Web IDL specification. If this pull request were to land, modifications to that specification to use the new EnsureFunctionName would be nice. Alternatively, we could provide a compatibility SetFunctionName abstract operation in ECMA-262 still. |
|
I wonder if it makes sense to delay merging this PR after we get a decision on #1372, because if this PR is first merged and at a later point #1372, #1372 will revert the name change from "EnsureFunctionName" back to "SetFunctionName" and also remove the HasOwnProperty check in SetFunctionName/EnsureFunctionName. |
|
I think #1373 does what I was looking to do with this. Any reason anyone sees to keep this open? |
No description provided.