-
Notifications
You must be signed in to change notification settings - Fork 93
Support elseif/else directive #328
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
base: master
Are you sure you want to change the base?
Conversation
…as structured Import type.
…conditional imports.
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.
[self review]
Except for Import, all nodes within IfDeclConfigSyntax are stored in IfMacroModel's entities.
However, import statement is not represented as one of them, rather it is represented as ImportContent.
| /// Represents a single clause in a conditional compilation block | ||
| struct Clause { | ||
| let type: ClauseType | ||
| let condition: String? // nil for #else | ||
| let entities: [(String, Model)] | ||
| } |
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.
[self review]
Now entities property is moved from IfMacroModel to IfMacroModel.Clause so that we can know which compiler directive stores them.
Because IfMacroModel self is also Model, this recursively stores.
sidepelican
left a comment
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.
I expect the review will take some time. I'll watch it at a more convenient time later.
|
|
||
| public typealias ImportMap = [String: [String: [String]]] | ||
| /// Structured import data parsed from a source file | ||
| public struct ParsedImports { |
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.
ParsedImports and ConditionalImportBlock appear to be representing the same thing.
Wouldn't it make sense to combine them if ClauseType includes a .topLevel ?
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.
ParsedImports can be removed, but I don't think adding topLevel case to ClauseType is better bacause topLevel is not a part of ClauseType.
I deleted ParsedImports in this commit 46a71d6.
Could you check if it gets better?
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.
handleImports has become more complex, but structurally it's improved—so the issue might be in handleImports. please give me some time to understand this.
| if let runHandler = runHandler { | ||
| runHandler() | ||
| } | ||
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.
Memo: tests are looks fine
Note
This PR is made after this closed PR that has some faults for parsing complicated if-compiler-directive.
Motivation
Mockolo didn't support elseif compiler directive as of v2.3.1.
There are some bugs that mockolo faces with:
Approach
This PR's main updates consist of two parts:
ImportMapmore structured by introducingParsedImportstype.IfMacroModeltoClausetype and store entities toClausetype rather thanIfMacroModel.Detail
ImportHandler
mockolo provides a feature that user can inject any import statement via mockolo CLI what we call
customImports.This is one of the reason that we can't collect all imports just leveraging swift-syntax.
Operation to merge all-imports is performed in
handleImportsfunction, and the function removes duplicated imports and recursively renders conditional import statements.IfMacroModel
This PR added
Clausetype toIfMacroModelthat representsif compiler directivein@mockableentity, and now entities are stored byClausetype.The order of
if-elseif-elseis decided by index ofIfConfigDeclSyntax.clauses.enumerated(), andcompilerDirectiveKeyin previous PR was not actually needed.