-
Notifications
You must be signed in to change notification settings - Fork 235
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 desugared directive #401
Conversation
…x_desugared_directive # Conflicts: # src/noAccessMissingMemberRule.ts # test/angularWhitespaceRule.spec.ts # test/templatesUsePublicRule.spec.ts
Awesome job! Will take a look as soon as possible. |
@wKoza there might be an issue in the testing package implementation. |
src/angularWhitespaceRule.ts
Outdated
const exprText = sf.substring(exprStart, exprEnd); | ||
|
||
let exprStart, exprEnd, exprText, sf; | ||
if (NgWalker.prop && ( NgWalker.prop.templateName === 'ngForOf' || NgWalker.prop.templateName === 'ngIf')) { |
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.
Maybe we can preserve the context (prop
) from a parent element visitor?
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.
Yes, this is an evil way. After a good night, now, we have our context the parent class sourceMappingVistor
. It's 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.
Finally, I modified SourceMappingVisitor.getSourcePosition()
method, which is more central. So, this fix the position problem in our tests.
src/angularWhitespaceRule.ts
Outdated
const exprText = sf.substring(exprStart, exprEnd); | ||
|
||
let exprStart, exprEnd, exprText, sf; | ||
if (NgWalker.prop && ( NgWalker.prop.templateName === 'ngForOf' || NgWalker.prop.templateName === 'ngIf')) { |
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.
Isn't this valid for all different templates? *
attribute prefix always desugars to <ng-template...></ng-template>
.
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.
Yes, a test on prop.templateName
should be enough.
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.
LGTM ✅
Closes #379
This solution is not graceful but it allows to recover a higher-level parent of the AST in our visitor. I think that this information was generally lacking. I did not find any other way to do it.
Another problem: We lose the correct place where the source code doesn't match the rule (see templatesUsePublicRule.spec.ts) in our tests. In my IDE (Webstrom), there is no position error.