Skip to content

Parse hashbang as a child of SourceFileSyntax #1979

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

Merged

Conversation

StevenWong12
Copy link
Contributor

Resolves #1825.

Instead of parsing hashbang into a token list, I think the easier way is to parse the whole as an identifier-like token since we don't care about the content of it.

Also, we could make use of Lexer.Cursor.advanceToEndOfLine to lex the first line as a hashbang token instead of doing this in the parser.

What do you think about it?

@StevenWong12 StevenWong12 requested a review from ahoppen as a code owner August 2, 2023 16:25
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looks great to me. One general note: I think the standard term here is Shebang instead of hashbang.

@StevenWong12 StevenWong12 force-pushed the parse_hashbang_as_child_of_sourfile branch from 8f39f7a to 99152e4 Compare August 6, 2023 15:24
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Oh, one more general comment: Could you add an assertParse text for the shebang? You might need to adjust BasicFormat.requiresNewline as well to make sure it adds a newline after the shebang.

@StevenWong12 StevenWong12 force-pushed the parse_hashbang_as_child_of_sourfile branch from 99152e4 to 605a5b8 Compare August 10, 2023 14:41
@StevenWong12
Copy link
Contributor Author

I think we already have an assertParse test in testHashbangLibrary1.😉

@ahoppen
Copy link
Member

ahoppen commented Aug 11, 2023

I think we already have an assertParse test in testHashbangLibrary1.😉

Oh, I didn’t see that. Thanks for pointing it out

@ahoppen
Copy link
Member

ahoppen commented Aug 11, 2023

@swift-ci Please test

@StevenWong12
Copy link
Contributor Author

I created a corresponding pr in swiftlang/swift-format#590. Could you trigger CI again?

@kimdv
Copy link
Contributor

kimdv commented Aug 12, 2023

@swift-ci please test

@allevato
Copy link
Member

swiftlang/swift-format#590

@swift-ci please test

@ahoppen ahoppen merged commit 3eb344c into swiftlang:main Aug 14, 2023
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.

Parse shebang in source file as an child of 'SourceFile'
4 participants