Include the position information for the Expression nodes in the Fhipath Parser #2774
Conversation
…pression types without impacting visitor implementations
…es from Constant to retain compatibility with existing code that might be using that type.
…und tripping more easily. This will reduce to nothing if not handled in a visitor
…ache Position information)
…n (standardizing whitespace) * Brackets can handle having extra whitespace around them * Brackets support left/right subtokens inside * FunctionCalls support left/right subtokens inside * Position information updated for function calls (and child expressions too) * Roundtripping fhirpath expressions unit tests
…he Expression objects and also handling comments in fhirpath expressions (they are skipped and not retained in the expression AST)
|
I need to recheck this against my other branch... fhirpath-position (which has the round-trip visualizer). |
|
Branch is now good for review. |
There was a problem hiding this comment.
There's a risk I am making comments about things we discussed before at the WGM, but it was so long ago, that I have forgotten our conclusions ;-)
In general, we should now be adding doccomments to any (new) public classes and other surface elements, so that's my major feedback. If possible, adding #nullable enable and specifying nullability markers would be nice, but sometimes that is a pandora's box.
src/Hl7.Fhir.Base/FhirPath/Expressions/FhirPathExpressionLocationInfo.cs
Show resolved
Hide resolved
Inclusion of an EchoVisitor (for future use) Move the Sub-Token class into it's own file
And more code comments.
… fhirpath language
|
I've completed all the changes requested. Diff here: |
| /// <summary> | ||
| /// Any leading whitespace or comments encountered immediately before this SubToken | ||
| /// </summary> | ||
| public IEnumerable<WhitespaceSubToken> leadingWhitespace { get; internal set; } |
There was a problem hiding this comment.
Should this be Pascal Cased?
| /// <summary> | ||
| /// Any trailing whitespace or comments encountered immediately after this SubToken | ||
| /// </summary> | ||
| public IEnumerable<WhitespaceSubToken> trailingWhitespace { get; internal set; } |
There was a problem hiding this comment.
Should this be Pascal Cased?
There was a problem hiding this comment.
Resolved in the other branch when that is merged in too.
| } | ||
| break; | ||
| case "String": | ||
| _result.Append($"'{t}'"); |
There was a problem hiding this comment.
@ewoutkramer ?
This line should really be handling the delimiting of the strings too.
Should the Functions.StringOperators.EscapeJson be the appropriate function to use there?
_result.Append("'" + Functions.StringOperators.EscapeJson(t) + "'");
Note: yes something does need to be done here, I found this round tripping all the search parameters/invariants.
There was a problem hiding this comment.
Yeah, if the goal of this visitor is to roundtrip to the original fhirpath as much as possible, it should escape stuff. Not easy to really roundtrip (as you cannot distinguish between allowed unicode characters and escaped unicodes anymore), but that would be good enough. So, it should "undo" the parsing of the string.
If this were C# we could leverage Roslyn:
return SyntaxFactory.LiteralExpression(SyntaxKind.StringLiteralExpression, SyntaxFactory.Literal(input)).ToFullString();
But I don't know if that's good enough.
| case "Integer": | ||
| _result.Append($"{t}"); | ||
| break; | ||
| case "Long": |
There was a problem hiding this comment.
You could combine all cases that do the same thing, to make clear they are doing the same thing!
| /// <summary> | ||
| /// Any leading whitespace or comments encountered immediately before this SubToken | ||
| /// </summary> | ||
| public IEnumerable<WhitespaceSubToken> leadingWhitespace { get; internal set; } |
|
@brianpos - note that both Gino and I had some review remarks. Build is currently failing because of a QA step: https://dev.azure.com/firely/firely-net-sdk/_build/results?buildId=38380&view=logs&j=558a83a0-5585-5f85-5265-cdf5febe3f62&t=a961ae7d-38e2-5a82-9685-674f5235ed2d, we have added new constructors which breaks binary compat. We're not striving to get to binary compat, and I think it's alright for compile compatibility. But I don't remember how to disable the message. @Kasdejong ? |
|
Gino's concerns are in the second branch that follows this one. I'll have another look at the constructors listed in the build. |
|
Should I just merge in the whitespace and comments capturing changes into this branch too now? |
I've reviewed those messages, It looks like its just that it wants the constructors that have the default location = null parameter split into seperate ones. |
Nah, let's do two PRs, this one is done & reviewed now. Just need to either split those constructors, or wait for Kas or Marten to tune that exception file. I could probably find out how, but after this FMG meeting I'll join them in going a short Ascension break, so it will be Monday before we get to it. |
…inary compatible with the previous release) (cherry picked from commit f13e409)
Description
The Expression class has a
Locationproperty which is intended to store the position information of each Expression object in the AST. This has never been populated (was expected to go via the constructor).This PR uses the sprache SetPos/Positioned() capability to inject that information while parsing.
The old anticipated approach has been retained, and leveraged for unit testing to manually populate the data.
It also adds support for skipping over comments in expressions.
Related issues
Resolves #2517
Testing
There are many new unit tests to verify the positioning information.
FirelyTeam Checklist