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

Editorial: more punctuation consistification issues #243

Closed
domenic opened this issue Dec 10, 2015 · 11 comments
Closed

Editorial: more punctuation consistification issues #243

domenic opened this issue Dec 10, 2015 · 11 comments

Comments

@domenic
Copy link
Member

domenic commented Dec 10, 2015

Abstract operation, internal method, and job declarations use variable spacing in their parameter lists. Sometimes Name(foo, bar), sometimes Name( foo, bar ), and sometimes Name ( foo, bar ), along with other variations that are presumably typos.

Record creation of various types uses different spacing as well: Record{[[foo]]: bar}, Record {[[foo]]: bar}, Record { [[foo]]: bar }, etc. Be sure not to just search for Record though; it applies to e.g. property attributes, "the record", and other record types like PropertyDescriptor or Completion.

Abstract operation/internal method/job declarations use a variety of ways of declaring optional parameters. Sometimes the comma is inside the brackets (Map.prototype.forEach), sometimes outside (Call, Construct), and most of the time optional arguments are not bracketed at all (CreateMutableBinding).

@ljharb
Copy link
Member

ljharb commented Mar 20, 2018

Is there a clear preference that we want? I'm happy to make everything consistent, but unless I'm told "pick your favorite" i'd rather go with the consensus.

@annevk
Copy link
Member

annevk commented Mar 20, 2018

From HTML:

Abstract operation definition
StructuredSerializeInternal ( value, forStorage [ , memory ] )
Records
{ [[Type]]: "Number", [[NumberData]]: value.[[NumberData]] }
Record { [[Key]], [[Value]] }
Abstract operation invocation
Let serializedKey be ? StructuredSerializeInternal(entry.[[Key]], forStorage, memory).

The above generally reads well to me and is what @domenic advocated we use in WHATWG if I remember correctly. Streams uses the same style.

@ljharb
Copy link
Member

ljharb commented Mar 20, 2018

To rephrase and make sure I'm reading properly:

  • Abstract ops: <operation name><space>(<space><argument list separated by ",<space>"><space>)
  • Records as pairs: {<space><pair of "<slot name>:<space><slot value>" separated by ",<space>"><space>}
  • Records as a list of a single keypair: Record<space>{<space><slot name>,<space><slot value><space>}
  • abstract op invocation: <operation name>(<argument list separated by ",<space>">), prefixed by ?<space> or !<space> as needed
  • end algorithm steps with <.>

?

That makes sense to me; I'll begin to prepare a patch.

@annevk
Copy link
Member

annevk commented Mar 20, 2018

That looks correct, except for <record name>. At least the example I quoted has "Record" there literally, linking to https://tc39.github.io/ecma262/#sec-list-and-record-specification-type. And looking at that a bit more it seems we're inconsistent in whether to precede a Record with such a literal.

@ljharb
Copy link
Member

ljharb commented Mar 20, 2018

Got it, updated my rephrasing.

ljharb added a commit to ljharb/ecma262 that referenced this issue Mar 21, 2018
ljharb added a commit to ljharb/ecma262 that referenced this issue Mar 21, 2018
ljharb added a commit to ljharb/ecma262 that referenced this issue Mar 21, 2018
@ljharb
Copy link
Member

ljharb commented Mar 21, 2018

I opened #1143 to handle most of it; I couldn't find a practical regex to use to locate "abstract op invocations" with improper spacing.

jmdyck added a commit to jmdyck/ecma262 that referenced this issue Mar 21, 2018
The discussion in issue tc39#243 doesn't say what to do
when a parameter list or argument list is empty.
It seems reasonable to infer that
- an empty parameter list (in a definition)
  should have one space between the parens, and
- an empty argument list (in an invocation)
  should have zero spaces between the parens.

Which is mostly what the spec does already.
This commit fixes a few spots where it doesn't.
ljharb added a commit to ljharb/ecma262 that referenced this issue Mar 21, 2018
ljharb added a commit to ljharb/ecma262 that referenced this issue Mar 21, 2018
ljharb added a commit to ljharb/ecma262 that referenced this issue Mar 21, 2018
ljharb pushed a commit to ljharb/ecma262 that referenced this issue Mar 21, 2018
The discussion in issue tc39#243 doesn't say what to do
when a parameter list or argument list is empty.
It seems reasonable to infer that
- an empty parameter list (in a definition)
  should have one space between the parens, and
- an empty argument list (in an invocation)
  should have zero spaces between the parens.

Which is mostly what the spec does already.
This commit fixes a few spots where it doesn't.
@bterlson
Copy link
Member

This is all fine by me except records, which don't need space between the record name and {. I don't think it helps readability and can definitely hurt depending on where line breaks end up. Yes ecmarkup could replace such occurrences with nbsp but it won't help in my editor (ecma262 source text does not use manual line wrapping).

@bterlson
Copy link
Member

Also, if there is an effort at a broad alignment, make sure to include the directed operation syntax .

@ljharb
Copy link
Member

ljharb commented Mar 22, 2018

@bterlson do you mean only for named records, or also for Record { vs Record{?

ljharb added a commit to ljharb/ecma262 that referenced this issue Apr 17, 2018
ljharb added a commit to ljharb/ecma262 that referenced this issue Apr 17, 2018
ljharb added a commit to ljharb/ecma262 that referenced this issue Apr 17, 2018
ljharb pushed a commit to ljharb/ecma262 that referenced this issue Apr 17, 2018
The discussion in issue tc39#243 doesn't say what to do
when a parameter list or argument list is empty.
It seems reasonable to infer that
- an empty parameter list (in a definition)
  should have one space between the parens, and
- an empty argument list (in an invocation)
  should have zero spaces between the parens.

Which is mostly what the spec does already.
This commit fixes a few spots where it doesn't.
ljharb added a commit to ljharb/ecma262 that referenced this issue Jun 4, 2018
ljharb added a commit to ljharb/ecma262 that referenced this issue Jun 4, 2018
ljharb added a commit to ljharb/ecma262 that referenced this issue Jun 4, 2018
ljharb pushed a commit to ljharb/ecma262 that referenced this issue Jun 4, 2018
The discussion in issue tc39#243 doesn't say what to do
when a parameter list or argument list is empty.
It seems reasonable to infer that
- an empty parameter list (in a definition)
  should have one space between the parens, and
- an empty argument list (in an invocation)
  should have zero spaces between the parens.

Which is mostly what the spec does already.
This commit fixes a few spots where it doesn't.
ljharb added a commit to ljharb/ecma262 that referenced this issue Jun 13, 2018
ljharb added a commit to ljharb/ecma262 that referenced this issue Jun 13, 2018
ljharb added a commit to ljharb/ecma262 that referenced this issue Jun 13, 2018
ljharb pushed a commit to ljharb/ecma262 that referenced this issue Jun 13, 2018
The discussion in issue tc39#243 doesn't say what to do
when a parameter list or argument list is empty.
It seems reasonable to infer that
- an empty parameter list (in a definition)
  should have one space between the parens, and
- an empty argument list (in an invocation)
  should have zero spaces between the parens.

Which is mostly what the spec does already.
This commit fixes a few spots where it doesn't.
@ljharb
Copy link
Member

ljharb commented Jun 13, 2018

With #1143, and a number of previous PRs, the only remaining item for this issue might be spacing around abstract operation invocations. Can someone review, and confirm/correct?

ljharb added a commit to ljharb/ecma262 that referenced this issue Jun 13, 2018
ljharb added a commit to ljharb/ecma262 that referenced this issue Jun 13, 2018
ljharb added a commit to ljharb/ecma262 that referenced this issue Jun 13, 2018
ljharb pushed a commit to ljharb/ecma262 that referenced this issue Jun 13, 2018
The discussion in issue tc39#243 doesn't say what to do
when a parameter list or argument list is empty.
It seems reasonable to infer that
- an empty parameter list (in a definition)
  should have one space between the parens, and
- an empty argument list (in an invocation)
  should have zero spaces between the parens.

Which is mostly what the spec does already.
This commit fixes a few spots where it doesn't.
@jmdyck
Copy link
Collaborator

jmdyck commented Sep 12, 2018

the only remaining item for this issue might be spacing around abstract operation invocations.

It looks like that was taken care of in early 2016 (e.g., 2fb8d08 and 690bcd9).

I think this issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants