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

[parser] Handle flow comments with leading spaces #9168

Merged
merged 5 commits into from Dec 14, 2018

Conversation

vikr01
Copy link
Contributor

@vikr01 vikr01 commented Dec 12, 2018

Q                       A
Fixed Issues? Fixes #9167
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? No
License MIT

Handle flow comments that have spaces or tabs between the end of the comment opening and the start of the flow annotation:

/*<spaces and tabs>:: type Foo = string; */
/*<spaces and tabs>flow-include type Bar = string; */
const foo/*<spaces and tabs>: string*/ = 'foo';

Copy link
Member

@danez danez left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. There are a lot of unnecessary changes in this PR, which should be excluded please.
Also the logic does no seem to be working correctly, as the flow tests fail and also the changes committed to some of the snapshots are wrong.

@danez
Copy link
Member

danez commented Dec 13, 2018

Are you planing on finishing this or otherwise I can have a look and see what is wrong?
The overall idea seems good to me and we should support spaces/tabs in flow comments similar to how flow does it.

@danez danez added PR: Bug Fix 🐛 A type of pull request used for our changelog categories PR: WIP area: flow pkg: parser labels Dec 13, 2018
@vikr01
Copy link
Contributor Author

vikr01 commented Dec 13, 2018

@danez Yeah if you could take a look I'd appreciate it, thanks.

@vikr01 vikr01 closed this Dec 13, 2018
@danez
Copy link
Member

danez commented Dec 13, 2018

I leave it open for now, so we do not forget :)

@danez danez reopened this Dec 13, 2018
@danez danez self-assigned this Dec 13, 2018
@vikr01 vikr01 force-pushed the flow-comments-leading-spaces branch from 24dc6fd to e43c940 Compare December 13, 2018 05:56
@vikr01 vikr01 force-pushed the flow-comments-leading-spaces branch from e43c940 to 07a417c Compare December 13, 2018 06:09
@vikr01 vikr01 force-pushed the flow-comments-leading-spaces branch from 07a417c to 3a34099 Compare December 13, 2018 06:13
@danez
Copy link
Member

danez commented Dec 14, 2018

Thanks for fixing all the stuff, I looked into why the flow tests still failed and fixed them.

@danez danez merged commit 72471af into babel:master Dec 14, 2018
@vikr01 vikr01 deleted the flow-comments-leading-spaces branch December 14, 2018 16:54
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: flow outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parser doesn't recognize flow comments with leading spaces
3 participants