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: iterator object != iterator record #1022

Closed
jmdyck opened this issue Oct 17, 2017 · 9 comments
Closed

Editorial: iterator object != iterator record #1022

jmdyck opened this issue Oct 17, 2017 · 9 comments
Labels

Comments

@jmdyck
Copy link
Collaborator

jmdyck commented Oct 17, 2017

ForIn/OfHeadEvaluation invokes EnumerateObjectProperties and returns its result.

But EnumerateObjectProperties returns an iterator object and ForIn/OfHeadEvaluation should be returning an iterator record (since aa16347, see issue #988 from @kmiller68).

So either:

  • ForIn/OfHeadEvaluation should be altered to construct an iterator record from the iterator object that EnumerateObjectProperties returns, or
  • EnumerateObjectProperties should be altered to return an iterator record.

(I think.)

@ljharb
Copy link
Member

ljharb commented Oct 17, 2017

I'd prefer the former, to avoid complicating the spec steps for Object.{keys,values,entries}.

@kmiller68
Copy link
Contributor

Whoops I missed this. Good catch @jmdyck! I agree with @ljharb the former seems like the better solution. I'm happy to make the change unless someone else is interested or objects.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Oct 17, 2017

I'd prefer the former, to avoid complicating the spec steps for Object.{keys,values,entries}.

I don't understand why changing EnumerateObjectProperties to return an iterator record would cause the spec steps for Object.{keys,values,entries} to change. They invoke EnumerableOwnProperties, which refers to EnumerateObjectProperties, but doesn't even invoke it, really. It seems like EnumerableOwnProperties could handle the change just by replacing the Iterator with the iterator record (in the Iterator that would be returned if the EnumerateObjectProperties internal method were invoked with _O_).

@kmiller68
Copy link
Contributor

kmiller68 commented Oct 17, 2017

I was thinking that it would be better to change ForIn/OfHeadEvaluation rather than EnumerableObjectProperties as changing EnumerableObjectProperties would mean that the example below needs to be changed. I'm mostly indifferent though.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Oct 17, 2017

EnumerableOwnProperties doesn't have an example. I think you mean EnumerateObjectProperties.

@kmiller68
Copy link
Contributor

Yeah, I copy pasted wrong the wrong word... That's what I get for commenting past my bed time... of 9pm.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Oct 17, 2017

Okay, the sample code in EnumerateObjectProperties seems like a valid reason.

@jmdyck
Copy link
Collaborator Author

jmdyck commented May 12, 2019

@kmiller68: Are you still happy to make the change?

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 30, 2020

It looks like PR #1969 fixed this.

@jmdyck jmdyck closed this as completed Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants