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

Improvement on error reporting of 'for await' loops #189

Merged
merged 3 commits into from
Oct 16, 2021
Merged

Improvement on error reporting of 'for await' loops #189

merged 3 commits into from
Oct 16, 2021

Conversation

adams85
Copy link
Collaborator

@adams85 adams85 commented Aug 19, 2021

A minor fix related to async iterations (see #183 (comment))

@adams85
Copy link
Collaborator Author

adams85 commented Aug 19, 2021

Ooops, looks like this needs some more work:

af79159#diff-92a67081d35facbe8921de0638d8eccd7f7204c8b99a656abc3af2ea750857c1R3045-R3055

Please don't merge this yet.

@@ -41,7 +41,7 @@ public Context()
public bool AllowIn;
public bool AllowStrictDirective;
public bool AllowYield;
public bool Await;
Copy link
Collaborator

Choose a reason for hiding this comment

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

even thought might not seem a good name, I think we should keep the naming that JS project has to make diffing and syncing easier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem, I'll revert it.

@adams85
Copy link
Collaborator Author

adams85 commented Aug 19, 2021

@lahma Ok, I went over it again, I think it's ready now.

Update: I squashed the corrections into the main commit to make it easier to review.

@lahma
Copy link
Collaborator

lahma commented Aug 22, 2021

I think we should wait for the Esprima change first, it also renames the context.await to context.isAsync which is more logical and will also reflect here (unfortunately clashing). I'd like to also see how this changes reported errors against JS test suite after those changes (ideally this would show improvements 😉 )

@lahma
Copy link
Collaborator

lahma commented Oct 16, 2021

Hey @adams85 I think we've forgotten this one, could you rebase? 🙂

@sebastienros
Copy link
Owner

Fixed the conflict from the GH UI, hopefully correctly.

Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

I think it's all good, might need some styling adjustment later on but test cases are covering now 👍🏻

@lahma lahma merged commit c16768b into sebastienros:main Oct 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants