Skip to content
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

Meta: integrate aspell for spelling checks #3335

Merged
merged 2 commits into from
Aug 7, 2024
Merged

Meta: integrate aspell for spelling checks #3335

merged 2 commits into from
Aug 7, 2024

Conversation

michaelficarra
Copy link
Member

Integrates aspell to check the spelling in spec.html. Currently just a package.json script, but if we want this enforced during CI, we should be able to do that, too. Updates a bunch of spellings for a variety of reasons. Commits should be reviewed individually.

@michaelficarra michaelficarra added editorial change meta editor call to be discussed in the next editor call labels May 25, 2024
@michaelficarra michaelficarra requested a review from a team May 25, 2024 00:20
@ljharb
Copy link
Member

ljharb commented May 28, 2024

"fulfill" has three L's. We don't generally strictly adhere to UK spellings, which imo is a good thing.

Also, "callbackfn" and "comparefn" aren't misspellings, and updating that sort of thing would force MDN to do a lot of updates.

@michaelficarra
Copy link
Member Author

We don't generally strictly adhere to UK spellings, which imo is a good thing.

We're very much expected to.

image

image

Also, "callbackfn" and "comparefn" aren't misspellings, and updating that sort of thing would force MDN to do a lot of updates.

I think the change is an improvement. I'm fine backing it out, but not on the grounds that MDN ties their documentation to the aliases we use in the spec. There is no stability guarantee for our alias name choices; we should always strive to use whatever we think communicates best.

@ljharb
Copy link
Member

ljharb commented May 28, 2024

ecma expects a lot of things that we elect not to do, for the betterment of tc39 and this spec :-)

@michaelficarra
Copy link
Member Author

Yes, and the editor group has quite explicitly agreed upon our divergences, such as:

image

But we've no good reason to diverge from the OED spelling requirement, IMO. We'll talk about it in editor call.

spec.html Outdated Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented May 29, 2024

Summarizing my preferences from the call:

  1. We'll review the changes in this PR individually. I lean towards using UK spellings just so there's an easy rule to follow.

  2. We should have a package.json script which runs against the current spec and extracts a list of words which are already used. Compound words, like AbstractOperationNames, should get split up in the obvious way. Then we can have a separate script which consumes that list as the dictionary. In CI, we can run the first script on the upstream version, and the second script on the PR, and that will tell us if the PR introduces any new potential misspellings (with possibly some false negatives, but whatever). Ideally it would leave these as comments on the PR, and should not be treated as a check which has to pass to land the change.

@bakkot bakkot removed the editor call to be discussed in the next editor call label Jun 4, 2024
Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

US vs UK spelling aside, I'm a big fan of the outerPrivEnv → outerPrivateEnv / iterNextObj → iteratorResult / ... changes! I've been spelling these words out when translating spec text into code and it usually leads to improved readability.

spec.html Outdated
@@ -47634,7 +47634,7 @@ <h1>GetGeneratorKind ( ): ~non-generator~, ~sync~, or ~async~</h1>
<emu-clause id="sec-generatoryield" type="abstract operation">
<h1>
GeneratorYield (
_iterNextObj_: an Object that conforms to the <i>IteratorResult</i> interface,
_iteratorResult_: an Object that conforms to the <i>IteratorResult</i> interface,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe iteratorResultToYield?

spec.html Outdated
<p>This method performs the following steps when called:</p>
<emu-alg>
1. Let _S_ be the *this* value.
1. Return ? CreateHTML(_S_, *"font"*, *"color"*, _color_).
1. Return ? CreateHTML(_S_, *"font"*, *"color"*, _colour_).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep these color. I'd prefer algorithm steps and aliases to be American spelling, like code.

spec.html Outdated
1. Let _shiftCount_ be ℝ(_rnum_) modulo 32.
1. Return the result of left shifting _lnum_ by _shiftCount_ bits. The mathematical value of the result is exactly representable as a 32-bit two's complement bit string.
1. Let _lNum_ be ! ToInt32(_x_).
1. Let _rNum_ be ! ToUint32(_y_).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like rFoo and lFoo over rfoo and lfoo.

spec.html Outdated
@@ -50440,8 +50440,8 @@ <h1>Changes to FunctionDeclarationInstantiation</h1>
1. When the |FunctionDeclaration| _f_ is evaluated, perform the following steps in place of the |FunctionDeclaration| Evaluation algorithm provided in <emu-xref href="#sec-function-definitions-runtime-semantics-evaluation"></emu-xref>:
1. Let _fEnv_ be the running execution context's VariableEnvironment.
1. Let _bEnv_ be the running execution context's LexicalEnvironment.
1. Let _fobj_ be ! _bEnv_.GetBindingValue(_F_, *false*).
1. Perform ! _fEnv_.SetMutableBinding(_F_, _fobj_, *false*).
1. Let _fObj_ be ! _bEnv_.GetBindingValue(_F_, *false*).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also do not like fObj over fobj. Overall not a fan of one lowercase letter then capitalized word.

spec.html Outdated
@@ -26881,7 +26881,7 @@ <h1>Example Cyclic Module Record Graphs</h1>
<ul>
<li>Evaluation must be only performed once, as it can cause side effects; it is thus important to remember whether evaluation has already been performed, even if unsuccessfully. (In the error case, it makes sense to also remember the exception because otherwise subsequent Evaluate() calls would have to synthesize a new one.)</li>
<li>Linking, on the other hand, is side-effect-free, and thus even if it fails, it can be retried at a later time with no issues.</li>
<li>Loading closely interacts with the host, and it may be desiderable for some of them to allow users to retry failed loads (for example, if the failure is caused by temporarily bad network conditions).</li>
<li>Loading closely interacts with the host, and it may be desirable for some of them to allow users to retry failed loads (for example, if the failure is caused by temporarily bad network conditions).</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Desiderable should be a word I tell you what.

spec.html Outdated
@@ -42954,8 +42954,8 @@ <h1>
1. Let _rawBytes_ be a List whose elements are the 8 bytes that are the IEEE 754-2019 binary64 format encoding of _value_. The bytes are arranged in little endian order. If _value_ is *NaN*, _rawBytes_ may be set to any implementation chosen IEEE 754-2019 binary64 format Not-a-Number encoding. An implementation must always choose the same encoding for each implementation distinguishable *NaN* value.
1. Else,
1. Let _n_ be the Element Size value specified in <emu-xref href="#table-the-typedarray-constructors"></emu-xref> for Element Type _type_.
1. Let _convOp_ be the abstract operation named in the Conversion Operation column in <emu-xref href="#table-the-typedarray-constructors"></emu-xref> for Element Type _type_.
1. Let _intValue_ be ℝ(_convOp_(_value_)).
1. Let _conversionOperation_ be the abstract operation named in the Conversion Operation column in <emu-xref href="#table-the-typedarray-constructors"></emu-xref> for Element Type _type_.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, kinda long. Maybe just op?

@@ -17851,7 +17851,7 @@ <h1>Rules of Automatic Semicolon Insertion</h1>

AsyncArrowFunction[In, Yield, Await] :
`async` [no LineTerminator here] AsyncArrowBindingIdentifier[?Yield] [no LineTerminator here] `=>` AsyncConciseBody[?In]
CoverCallExpressionAndAsyncArrowHead[?Yield, ?Await] [no LineTerminator here] `=>` AsyncConciseBody[?In] #callcover
CoverCallExpressionAndAsyncArrowHead[?Yield, ?Await] [no LineTerminator here] `=>` AsyncConciseBody[?In] #callCover
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't worry about purely internal stuff like this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, here and below, prefer no change to the ids.

@@ -7121,7 +7121,7 @@ <h1>

<emu-clause id="sec-createiterresultobject" type="abstract operation">
<h1>
CreateIterResultObject (
CreateIteratorResultObject (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth giving the clause an ID to match the new name? With an oldid, of course.

spec.html Show resolved Hide resolved
spec.html Outdated
@@ -46873,7 +46869,7 @@ <h1>Promise Resolve Functions</h1>

<emu-clause id="sec-fulfillpromise" type="abstract operation">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New ID?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a need to always update the IDs to match the text. We're just going to have to oldid it anyway.

spec.html Outdated Show resolved Hide resolved
@michaelficarra michaelficarra force-pushed the spelling branch 3 times, most recently from 733031d to aab4308 Compare July 26, 2024 01:40
@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Jul 26, 2024
@@ -17851,7 +17851,7 @@ <h1>Rules of Automatic Semicolon Insertion</h1>

AsyncArrowFunction[In, Yield, Await] :
`async` [no LineTerminator here] AsyncArrowBindingIdentifier[?Yield] [no LineTerminator here] `=>` AsyncConciseBody[?In]
CoverCallExpressionAndAsyncArrowHead[?Yield, ?Await] [no LineTerminator here] `=>` AsyncConciseBody[?In] #callcover
CoverCallExpressionAndAsyncArrowHead[?Yield, ?Await] [no LineTerminator here] `=>` AsyncConciseBody[?In] #callCover
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, here and below, prefer no change to the ids.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@@ -50790,11 +50786,11 @@ <h1>String.prototype.fixed ( )</h1>
</emu-annex>

<emu-annex id="sec-string.prototype.fontcolor">
<h1>String.prototype.fontcolor ( _color_ )</h1>
<h1>String.prototype.fontcolor ( _colour_ )</h1>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate this very much but I guess it's fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the argument has always been named "color", imo this isn't just spec-internal and shouldn't be changed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we were back and forth on this one. Definitely open to changing it back in a follow-up, but the PR was in a good-enough state for now.

@michaelficarra michaelficarra added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels Aug 7, 2024
@ljharb ljharb changed the title integrate aspell for spelling checks Meta: integrate aspell for spelling checks Aug 7, 2024
@ljharb ljharb merged commit 4bc2d8d into main Aug 7, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change meta ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants