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

Use typechecker to determine property types #10558

Merged
merged 8 commits into from
Nov 19, 2021
Merged

Conversation

jackofdiamond5
Copy link
Member

@jackofdiamond5 jackofdiamond5 commented Nov 18, 2021

Closes #10572

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@valadzhov valadzhov added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Nov 18, 2021
@damyanpetev damyanpetev added the squash-merge Merge PR with "Squash and Merge" option label Nov 19, 2021
@@ -45,7 +50,9 @@ export const getIdentifierPositions = (source: string | ts.SourceFile, name: str
return false;
}
}
return node.text === name;
// for methods the node.text will not contain characters like ()
const cleanName = name.match(/\w+/g)[0] || name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackofdiamond5 for future improvement: this can probably be resolved with some kind check and likely the method nodes will have name or something

ClosingParenthesis = ')',
MemberAccess = '.',
Question = '?'
}

export class MemberInfo implements Pick<tss.DefinitionInfo, 'name' | 'fileName'> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while this is related to members, this isn't the member info - it's still the definition info for whatever we looked up at a position and it's usually the container type of the member instead :) Don't much care for the name ATM, just saying it's not quite right.

projects/igniteui-angular/migrations/common/tsUtils.ts Outdated Show resolved Hide resolved
@Lipata Lipata requested a review from damyanpetev November 19, 2021 13:14
@valadzhov valadzhov added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels Nov 19, 2021
@Lipata Lipata merged commit c195aa4 into master Nov 19, 2021
@Lipata Lipata deleted the bpenkov/typechecking branch November 19, 2021 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migrations squash-merge Merge PR with "Squash and Merge" option version: 13.0.x ✅ status: verified Applies to PRs that have passed manual verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use TypeChecker to resolve types when migrating TS properties
7 participants