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

fixes #1133: replace some uses of "idempotent" with alternative wording #1363

Merged
merged 1 commit into from Dec 11, 2019

Conversation

@michaelficarra
Copy link
Member

michaelficarra commented Nov 29, 2018

Fixes #1133. The other uses of "idempotent" (to mean "if applied to any of its own outputs, it behaves as the identity function") were left intact.

@michaelficarra michaelficarra requested a review from bterlson Nov 29, 2018
@ljharb
ljharb approved these changes Nov 29, 2018
Copy link
Member

ljharb left a comment

Seems clearer to me.

@ljharb ljharb requested review from zenparsing and tc39/ecma262-editors Nov 29, 2018
michaelficarra added a commit to michaelficarra/ecma262 that referenced this pull request Nov 29, 2018
ljharb added a commit to michaelficarra/ecma262 that referenced this pull request Feb 21, 2019
…lly defined

 - aligned with tc39#1363
ljharb added a commit to michaelficarra/ecma262 that referenced this pull request Feb 21, 2019
…lly defined

 - aligned with tc39#1363
ljharb added a commit to michaelficarra/ecma262 that referenced this pull request Feb 22, 2019
…lly defined

 - aligned with tc39#1363
ljharb added a commit to michaelficarra/ecma262 that referenced this pull request Feb 22, 2019
…lly defined

 - aligned with tc39#1363
@ljharb ljharb removed the request for review from bterlson Feb 28, 2019
@ljharb ljharb self-assigned this Mar 7, 2019
@michaelficarra michaelficarra force-pushed the michaelficarra:GH-1133 branch from 5d3e602 to 65fa26f Mar 26, 2019
@littledan

This comment has been minimized.

Copy link
Member

littledan commented Mar 26, 2019

I'm not sure about this change; I prefer the "idempotent" wording, for the reasons described in #1482 (comment) . However, I don't feel very strongly, and I'm fine to adopt the "deterministic" wording if everyone else likes that better. I'll land that change in #1482 if this one lands.

@allenwb

This comment has been minimized.

Copy link
Member

allenwb commented Mar 26, 2019

I'm with @littledan but stronger. "Determinist" is not an appropriate substitute for for "idempotent" in these cases.

@claudepache

This comment has been minimized.

Copy link
Contributor

claudepache commented Mar 26, 2019

“idempotent with respect to the program state”?

@ljharb ljharb force-pushed the michaelficarra:GH-1133 branch from 65fa26f to 28f5595 Apr 10, 2019
ljharb added a commit to michaelficarra/ecma262 that referenced this pull request Apr 10, 2019
@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Apr 17, 2019

@michaelficarra are you comfortable with the above suggestion?

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented May 17, 2019

@ljharb No, not at all. I still fail to see how "deterministic with respect to its parameters" is insufficient to describe these scenarios. If the parameters are the same across many calls, the function's behaviour will be the same. We shouldn't continue to use "idempotent" in two very different ways.

@bathos

This comment has been minimized.

Copy link
Contributor

bathos commented May 17, 2019

Deterministic, even with the ‘with respect to’ qualification, I would usually expect to mean only that it introduces no entropy to the application state, which doesn’t necessarily imply the function returns the same value, only that the value is predictable. Maybe if the qualifier were ‘with respect only to’?

@TimothyGu

This comment has been minimized.

Copy link
Member

TimothyGu commented May 20, 2019

How about just remove this sentence? The rest of the paragraphs explain what are meant in these contexts anyway, though we should still keep "if it completes normally" around.

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented May 21, 2019

@bathos IME, "with respect to" used in this way always means "with respect only to". But I wouldn't refuse if that makes it clearer for some.

@TimothyGu That is an okay compromise if the editors don't like the proposed wording.

@bathos

This comment has been minimized.

Copy link
Contributor

bathos commented May 28, 2019

@michaelficarra You’re right — I suppose I figured the ‘only’ might shift emphasis in a way that made the intended meaning clearer, but maybe that’s not true.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented May 28, 2019

@littledan, @allenwb - you two seem to still be against this change; do you have additional thoughts, or has your opinion changed?

@littledan

This comment has been minimized.

Copy link
Member

littledan commented May 29, 2019

Has something changed in this PR? My feeling is still what I said in #1363 (comment) .

@bakkot

This comment has been minimized.

Copy link
Contributor

bakkot commented May 29, 2019

FWIW I am strongly in favor of this change for the reasons given in the OP of #1133. However, if people are getting tripped up by the use of "deterministic with respect to its parameters" (for reasons which I do not entirely understand), we could just not pick a word for this, and instead modify the three lines changed by this PR to something like

ResolveExport is side-effect free. Each time it is called with a specific exportName, resolveSet pair as arguments it must return the same result. An implementation might choose to pre-compute or cache the ResolveExport results for the [[Exports]] of each module namespace exotic object.

Each time this operation is called with a specific exportName, resolveSet pair as arguments it must return the same result if it completes normally.

Each time this operation is called with a specific referencingModule, specifier pair as arguments it must return the same Module Record instance if it completes normally.

@zenparsing

This comment has been minimized.

Copy link
Member

zenparsing commented May 29, 2019

@bakkot I like it.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented May 29, 2019

That seems equally correct but less concise - but if it’s more widely acceptable than it seems like a good improvement.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jun 6, 2019

@littledan what do you think about this suggestion?

@littledan

This comment has been minimized.

Copy link
Member

littledan commented Jun 25, 2019

@bakkot That wording seems fine to me.

@michaelficarra michaelficarra force-pushed the michaelficarra:GH-1133 branch from 28f5595 to 978649d Jul 29, 2019
michaelficarra added a commit to michaelficarra/ecma262 that referenced this pull request Jul 29, 2019
@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Jul 29, 2019

@tc39/ecma262-editors Rebased and updated to use the above suggested wording.

@michaelficarra michaelficarra force-pushed the michaelficarra:GH-1133 branch from 978649d to 5980b5c Jul 30, 2019
spec.html Outdated Show resolved Hide resolved
@michaelficarra michaelficarra requested review from syg and bakkot Nov 8, 2019
@bakkot
bakkot approved these changes Nov 9, 2019
michaelficarra added a commit to michaelficarra/ecma262 that referenced this pull request Nov 9, 2019
@michaelficarra michaelficarra force-pushed the michaelficarra:GH-1133 branch from 5980b5c to 8b7a87c Nov 9, 2019
@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Nov 9, 2019

Rebased.

@syg
syg approved these changes Dec 4, 2019
…#1363)

Fixes #1133.
@ljharb ljharb force-pushed the michaelficarra:GH-1133 branch from 8b7a87c to ae2d1a8 Dec 11, 2019
@ljharb ljharb merged commit ae2d1a8 into tc39:master Dec 11, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/ecma262-snapshots/deploy-preview Deploy preview ready!
Details
@michaelficarra michaelficarra deleted the michaelficarra:GH-1133 branch Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.