Skip to content

Fix vect parsing with newline before comma#60

Merged
pfitzseb merged 3 commits intomainfrom
sp/fix-vect-newline
Aug 24, 2022
Merged

Fix vect parsing with newline before comma#60
pfitzseb merged 3 commits intomainfrom
sp/fix-vect-newline

Conversation

@pfitzseb
Copy link
Copy Markdown
Member

Fixes #59.

@pfitzseb pfitzseb requested a review from c42f August 22, 2022 14:03
@oscardssmith
Copy link
Copy Markdown
Member

Looks good. Merge?

@pfitzseb
Copy link
Copy Markdown
Member Author

I'll merge this tomorrow morning, unless Chis chimes in and says this is the wrong fix :)

The test is currently a bit awkward, because SyntaxNode/Expr conversion can't handle unexpected ,s. Would like to fix that.

Copy link
Copy Markdown
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

Looks good thanks. Let's add such error cases to the tests as s-expressions along with the happy cases.

Comment thread test/parser.jl Outdated
Comment thread src/parser.jl
@c42f
Copy link
Copy Markdown
Member

c42f commented Aug 23, 2022

The test is currently a bit awkward, because SyntaxNode/Expr conversion can't handle unexpected

Ah yeah I didn't see this comment before I wrote the review. Precisely :)

@pfitzseb
Copy link
Copy Markdown
Member Author

Tests look a lot nicer now. We're now just emitting an ErrorVal when encountering unknown node kinds, which may be a bad idea, but at least allows us to recover from the corrupted tree we produce further down :P

@pfitzseb pfitzseb requested a review from c42f August 23, 2022 11:17
@pfitzseb
Copy link
Copy Markdown
Member Author

This good to go, or do you think emitting ErrorVals instead of actually erroring is going to be an issue further down the line?

Copy link
Copy Markdown
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

The FIXME with ErrorVal makes me a bit uncomfortable (does seem wrong). But this is still an improvement so I think we can merge and fix that later.

@pfitzseb pfitzseb merged commit 66acd86 into main Aug 24, 2022
@pfitzseb pfitzseb deleted the sp/fix-vect-newline branch August 24, 2022 08:03
c42f pushed a commit to JuliaLang/julia that referenced this pull request Oct 17, 2025
…vect-newline

Fix vect parsing with newline before comma
topolarity pushed a commit to JuliaLang/julia that referenced this pull request Nov 14, 2025
…vect-newline

Fix vect parsing with newline before comma
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.

Incorrect parsing of array literal with newline before comma

3 participants