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

Fix completion record for labeled statement #13084

Merged
merged 1 commit into from Apr 20, 2021

Conversation

addaleax
Copy link
Contributor

@addaleax addaleax commented Apr 1, 2021

{labelAlthoughIMeantItToBeAKey: value} strikes again :)

Q                       A
Fixed Issues?
Patch: Bug Fix? yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? (where are the tests for getCompletionRecords?)
Documentation PR Link
Any Dependency Changes?
License MIT

@babel-bot
Copy link
Collaborator

babel-bot commented Apr 1, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/45437/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 1, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit bf1e8fe:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Apr 1, 2021
@nicolo-ribaudo
Copy link
Member

Related - #13030. Tt conflicts, but it looks like it will be easy to fix the conflict since modified line would be https://github.com/babel/babel/pull/13030/files#diff-6828b1e955c2a5ac30a60be81472c92a36c5b90605ba94a4ebfcaedbbc41b43dR213

where are the tests for getCompletionRecords

I think we only test it indirectly with do expressions, but if you can also add a new test in https://github.com/babel/babel/blob/main/packages/babel-traverse/test/family.js that would be appreciated!

@addaleax
Copy link
Contributor Author

addaleax commented Apr 1, 2021

@nicolo-ribaudo Ah, thanks :) This isn’t urgent for me, I think it would be fine to wait until the other PR is merged.

@hzoo
Copy link
Member

hzoo commented Apr 1, 2021

Since we don't have any getCompletionRecords tests, we could also add the tests for do expressions as mentioned https://github.com/babel/babel/tree/main/packages/babel-plugin-proposal-do-expressions/test/fixtures/do-expressions as a substitute?

// label.js
expect(do {
  a: 1
}).toBe(1);

expect(do {
  a: if (true) {
    "bar";
  }
}).toBe("bar");

@nicolo-ribaudo
Copy link
Member

@addaleax The other PR has been merged!

@addaleax addaleax force-pushed the completion-records-labeled-statement branch 3 times, most recently from 2f089ee to 67d5842 Compare April 20, 2021 11:59
`{labelAlthoughIMeantItToBeAKey: value}` strikes again :)
@addaleax addaleax force-pushed the completion-records-labeled-statement branch from 67d5842 to bf1e8fe Compare April 20, 2021 12:10
@addaleax
Copy link
Contributor Author

@nicolo-ribaudo Thank you, should be working now :)

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

awesome!

@JLHwung JLHwung merged commit b971e00 into babel:main Apr 20, 2021
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 21, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Do Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants