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 incorrect parsing of binary expressions (fixes #153) #158

Merged

Conversation

ketkarameya
Copy link
Contributor

Should I apply this similar pattern for other binary operators too? Or is there a nicer way to do it :O ?

@ketkarameya ketkarameya changed the title Fix/incorrect parsing conjunctions Fix/incorrect parsing conjunctions #153 Apr 19, 2022
@ketkarameya ketkarameya changed the title Fix/incorrect parsing conjunctions #153 Fix/incorrect parsing conjunctions Apr 19, 2022
@ketkarameya ketkarameya changed the title Fix/incorrect parsing conjunctions Fix the incorrect parsing of binary expressions Apr 19, 2022
@ketkarameya ketkarameya changed the title Fix the incorrect parsing of binary expressions Fixes #153 - Incorrect parsing of binary expressions Apr 19, 2022
@ketkarameya ketkarameya changed the title Fixes #153 - Incorrect parsing of binary expressions Fix incorrect parsing of binary expressions (fixes #153) Apr 19, 2022
@alex-pinkus
Copy link
Owner

Interesting, it looks like this causes a parse failure at https://github.com/groue/GRDB.swift/blob/4472d3a60ec4cc383503eba5072ce17f781b9ddc/GRDB/QueryInterface/SQLGeneration/SQLQueryGenerator.swift#L86 — possibly related to the binary expression that starts on line 77 if it changes the way that gets parsed?

@ketkarameya
Copy link
Contributor Author

ketkarameya commented Apr 19, 2022

I wonder why this fails :| I tried a very basic if try which throws an error.

func doSomething() {
    if try something {
        return a
    }
    return defaultValue
}

I couldn't find where if try is handled. Is it missing?
Would we need something similar to _if_let_binding?
I observed that this scenario worked before my change, so it is this new change that has caused this failure :|


I saw that just try works alright. if try is the problem.

@alex-pinkus
Copy link
Owner

Nice, you got it working! I blame swift for this ambiguity — whoever thought that “if statements without parentheses” and “trailing closures” belonged in the same language together were clearly not thinking for the long term.

@alex-pinkus
Copy link
Owner

This all looks good to me, so I’m happy to merge it later tonight. If you’d like, you can also amend to clean up the git commits and then git push origin with --force so that the reverts don’t show up in the history. Just let me know whether you’d like to do that and I can merge it after!

@ketkarameya ketkarameya force-pushed the fix/incorrect-parsing-conjunctions branch from 504a21d to d807c5d Compare April 20, 2022 05:00
@ketkarameya
Copy link
Contributor Author

Done. I have cleaned the history. Please merge the request. :)

@alex-pinkus alex-pinkus merged commit d1f5630 into alex-pinkus:main Apr 20, 2022
@alex-pinkus
Copy link
Owner

Done! Thank you so much for fixing this!

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.

2 participants