Editorial: rename single-letter and initial-capital aliases#3789
Conversation
|
The rendered spec preview for this PR is available as a single page at https://tc39.es/ecma262/pr/3789 and as multiple pages at https://tc39.es/ecma262/pr/3789/multipage . |
There was a problem hiding this comment.
big fan except for these:
prop,propKey,propertyKey- not sure why you turnedPinto three variants?pk- should be whatever the above gets unified torx- awfully shortened to avoid conflicts withregexp, a clear sign that something went wrong naming wise
I also don't quite like obj, funcObj, desc, and str (why numberString and not numberStr then?) but won't block those.
|
@linusg We're already fairly inconsistent with how short we shorten our aliases. I'm happy to make your suggested changes, but if we want to actually be consistent with how we abbreviate words for alias names (and probably AO names?), that'd be another whole effort in itself. |
|
Naming is hard 🙂 Happy with a follow-up but would love to see the two letter ones disappear now. |
| <ol> | ||
| <li> | ||
| _P_ is a writable data property. A non-configurable writable data property can be changed into a non-configurable non-writable data property. | ||
| _propertyKey_ is a writable data property. A non-configurable writable data property can be changed into a non-configurable non-writable data property. |
There was a problem hiding this comment.
This reads really weird, but it's just because of existing type confusion in the prose. We should follow up to make the prose work better.
There was a problem hiding this comment.
Also in the other internal methods
|
the one-letter variables read much more nicely to me uppercase than lowercase, fwiw. |
|
Okay I've scrolled through the diff and fixed all the errors I could find. I also updated the summary, which turned out to be a very effective way to find erroneous renames ("why is the |
|
Did a full manual pass over the diff.
|
|
@linusg Thank you for the review. Comments addressed. |
linusg
left a comment
There was a problem hiding this comment.
I looked at the additional commits and they seem good, but I'd like to do another pass after someone else has reviewed this before approving. There's also a conflict 🙂
|
With a PR this large and spread out, it's going to be hard to keep it conflict-free. So @tc39/ecma262-editors let's try to get it reviewed soon if we can. |
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
Note: I only reviewed your changes and code around them. I didn't go through untouched text yet.
| @@ -37555,29 +37555,29 @@ <h1> | |||
| </emu-alg> | |||
| <emu-grammar>Alternative :: Alternative Term</emu-grammar> | |||
| <emu-alg> | |||
| 1. Let _m1_ be CompileSubpattern of |Alternative| with arguments _rer_ and _direction_. | |||
| 1. Let _m2_ be CompileSubpattern of |Term| with arguments _rer_ and _direction_. | |||
| 1. Let _m1_ be CompileSubpattern of |Alternative| with arguments _regexpRecord_ and _direction_. | |||
There was a problem hiding this comment.
This section is still full of pretty useless names, some of which are one letter some of which are two letters but they are just the initial of two words put together (_m1_, _m2_, _d_, _c_, _q_, _xe_, _ye_, _cv_ ...).
I don't know much about our regexp spec to have suggestions, maybe leave it as a follow-up since they are a lot.
There was a problem hiding this comment.
I think I'd prefer to leave the matcher AOs to a follow-up. The goal of this proposal is not to completely eliminate abbreviated aliases, but to reduce their usage where it would help with readability. And I don't think replacing m1/m2 with something longer would help. Maybe some of the others would, though.
| 1. Let _rer_ be the RegExp Record { [[IgnoreCase]]: _i_, [[Multiline]]: _m_, [[DotAll]]: _s_, [[Unicode]]: _u_, [[UnicodeSets]]: _v_, [[CapturingGroupsCount]]: _capturingGroupsCount_ }. | ||
| 1. Set _obj_.[[RegExpRecord]] to _rer_. | ||
| 1. Set _obj_.[[RegExpMatcher]] to CompilePattern of _parseResult_ with argument _rer_. | ||
| 1. Let _regexpRecord_ be the RegExp Record { [[IgnoreCase]]: _i_, [[Multiline]]: _m_, [[DotAll]]: _s_, [[Unicode]]: _u_, [[UnicodeSets]]: _v_, [[CapturingGroupsCount]]: _capturingGroupsCount_ }. |
There was a problem hiding this comment.
These one-line vars are good.
There was a problem hiding this comment.
I'm assuming you mean one-letter? If so, I agree, which is why I've left them.
| 1. Set _value_ to CanonicalizeKeyedCollectionKey(_value_). | ||
| 1. For each element _e_ of _S_.[[SetData]], do | ||
| 1. For each element _e_ of _set_.[[SetData]], do |
There was a problem hiding this comment.
There are a bunch of loops across the spec where we keep "For each element e of". Pointing it out but I don't have a better suggestion, maybe 1 letter is fine since it's just immediately used in the line after.
There are also still many places where we use |
There is a whole bunch of |
jmdyck
left a comment
There was a problem hiding this comment.
There are problems with renaming single-letter aliases to have the same name as an alias that already exists in the same algorithm.
|
Thanks for the review @nicolo-ribaudo. I pushed up a bunch of commits addressing your feedback. |
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
This is now good enough for me :)
There are potential follow-up opportunities but let's merge this first.
|
@linusg Did you want to do another review, or should I mark this one as ready to merge? |
|
i'll do another review today. |
linusg
left a comment
There was a problem hiding this comment.
- PrivateElementFind, PrivateFieldAdd, PrivateGet, PrivateSet:
propertyKey->privateName(used to beP) - there's precedent in https://tc39.es/ecma262/#sec-relational-operators-runtime-semantics-evaluation - CreateListIteratorRecord:
e->value(used to beE) - SuperCall evaluation: having
funcandfuncObjis not great (one isthis, the other one the super constructor) - PropertyDestructuringAssignmentEvaluation, IteratorDestructuringAssignmentEvaluation:
v->value - RegExp:
pattern->regexpOrPatternwould resolve some awkwardness like we did in the String.prototype methods, havingpatternandpatternSourceis weird - IsPromise:
promise->arg/argument/valuelike in most of https://tc39.es/ecma262/#sec-testing-and-comparison-operations. I think speculative naming is good when the first step is validation and the expectation is that a certain object type is passed, but here it's completely neutral.
There's some other stuff that I don't like but I'll submit that as follow-ups.
|
@michaelficarra this PR has 91 commits. should they just all be squashed into one? |
|
yeah I think so |
Fixes #3119. Generated summary of renames:
AarrayAcaseClausesAcharSetAdeletedArrayAnewArrayAresultArrayaintegerPartARagentRecordAttributesattributesBblockCompletionBblockResultBfirstOctetBotherSetBsecondCaseClausesbfractionalPartBCboundTargetBodybodyCactiveFuncCcatchResultCclauseCcodeUnitCcompletionCconstructorCremainingSetCspeciesConstructorCstmtCompletionCvalueccontinuecexponentSignCurrentcurrentDdeletableDdescDeventBDrangeSetdexponentDigitsDclRecdeclarativeRecordDescdescEeEeventEeventAEitemEouterEnveexponentematchEndExtensibleextensibleFactiveFuncFboundFuncFclosureFconstructorFconstructorFunctionFeventCFfieldNameFfinallyResultFflagsFfuncFfuncNameFfuncObjFobjFthisValueffractionCountGgeneratorGobjicaptureIndexisearchStartiwriteIndexInputinputInputLengthinputLengthjmatchIndexKkeyskdigitCountksourceIndexLlengthLlowercaseStringLwaitersLeftFirstleftFirstMmapMmoduleMtargetModuleMweakMapmdigitStringmsignificandNnNnamencaptureIndexnintSignificandnintValuenmatchCountnnextIndexnresultIndexnstartIndexN2n2OiteratorObjOobjOthisValueOvalueObjobjObjRecobjectRecordOctetsoctetsPpPparameterStringPpatternPpatternSourcePpropertyKeyPpropertyNamePprotoPproxyplastMatchEndpmatchPositionpprecisionCountppropertyKeyParameterListparameterListPipropertyKeyPkpropertyKeyproppropertyKeyPropertiespropertiesPropertyDescriptorpropertyDescriptorPropertyListpropertyListpropKeypropertyKeyqsearchIndexRcompletionRobjRrRradixMVRreadEventRregexpRresultRseparatorStrRthisValuerreadIndexrealmCconstructorRealmReceiverreceiverReplacerFunctionreplacerFunctionrerregexpRecordrxregexpScharSetSelementStrSescapedPatternSeventASisStrictSobjectSetSsegmentSsetSspeciesSstrSthisValueStrimmedStringSvaluesSwaitersSweakSetssignSortComparesortCompareStrictisStrictSweventBTsubstringTtrimmedStringTargettargetThrowthrowVcodePointViterationResultVobjVprotoVreferenceRecordVresultValueVthisValueVvaluevvalueWnewValueWreferenceRecordWvalueWwaiterRecordWwriteEventWLwaiterListWswritesXownPropertyxmatchStatexnumberValuexsourceZnumberStringzmatchResultzzeroPadDisclaimer: I used AI in the process of creating this PR.