-
Notifications
You must be signed in to change notification settings - Fork 301
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
Recent Grammar issues #6196
Recent Grammar issues #6196
Conversation
✅ Build Rubberduck 2.5.9.6317 completed (commit 80ad5e5400 by @tommy9) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #6196 +/- ##
==========================================
+ Coverage 97.34% 97.58% +0.24%
==========================================
Files 4 4
Lines 413 413
Branches 28 28
==========================================
+ Hits 402 403 +1
Misses 6 6
+ Partials 5 4 -1 |
Thanks for working on the mess I caused. I would have liked to do it myself for a whole, but my real work is in crunch mode at the moment. I think the fix actually does not address the full problem with the single line if statement. It certainly removes the rule confusion on multi-line if statements, but it does not address the resolver issue on single line if statements with empty then clauses. I think that is caused by me forgetting to put a null check into the method handling single line if statement contexts in the resolver when I made the one part optional. I believe that still needs to be added. |
Thanks, I didn't think to change the lack of null checking because the grammar fix should mean it there should never be an empty else clause and an empty then clause. However, who knows what odd code could end up taking that path, so I agree it is safest to add a check in. I'll update the pull request. |
✅ Build Rubberduck 2.5.9.6318 completed (commit 604e240b47 by @tommy9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for dealing with this.
@retailcoder something is wrong with the release URL. When I try to download the updated build from the website for testing, I get a 404 on the redirect to Github. The link with the 404 is: It should be: (without the asterisks, of course :-) ) |
Argh, my bad - with the web API being down I'm manually inserting the records in the database... I messed that one up, will update over lunch (30-60 minutes) |
Fixed! |
Fixes for #6187, #6194
Single line if statements had been made too flexible, in particular the rule ifWithEmptyThen where all the elements following the THEN were optional, letting the singleLineIfStmt match what is really an ifStmt:
ifWithEmptyThen : IF whiteSpace? booleanExpression whiteSpace? THEN whiteSpace? emptyThenStatement? singleLineElseClause?;
Now the rule requires at least one of emptyThenStatement or singleLineElseClause.
The user defined type member rule has been modified to allow the asTypeClause to be omitted but only if the member is an array. This ensures valid (though not defined in the language specification) examples through as in #6187
optionalArrayClause : ((arrayDim whiteSpace)? asTypeClause | arrayDim);