-
Notifications
You must be signed in to change notification settings - Fork 6
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(aliasing): added new resolvedName property to typeIdentifier object for import aliased declrations #36
fix(aliasing): added new resolvedName property to typeIdentifier object for import aliased declrations #36
Conversation
…ct for import aliased declrations Signed-off-by: sanketshevkar <[email protected]>
Pull Request Test Coverage Report for Build 12807110955Details
💛 - Coveralls |
If you use |
…ct for import aliased declrations Signed-off-by: sanketshevkar <[email protected]>
lib/metamodelutil.js
Outdated
}); | ||
} else { | ||
(modelFile.declarations || []).forEach((decl) => { | ||
// What is this used for? |
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.
Why add a comment here? This comment doesn't explain the code or notify anything. Maybe just add a comment to the PR? Or raise it in Discord?
Could you remove it before you merge this?
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.
The code is adding types defined in the current CTO to the types table.
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.
Sorry I had forgot to remove the comment from the older PR.
@ekarademir I thought it works only if the issues are logged in same repo as the PR. I'm not sure though. I'll make an update in either case. Also I've fixed the coverage drop by fixing the aliasing test. |
PartialyCloses accordproject/concerto#972
Added to new optional
resolvedName
property toTypeIdenifier
. This is to preserve the aliasedname
property which will have the local name for the aliased type.resolvedName
will contain the origonal type name. It will get added only for the declrations ot types that "imported and aliased".With the current implementation the value of the
name
property is different for a normal ast (local name) vs a resolved ast (original type name). This can be very confusing.The part that would close this issue would be adding unit tests to cover the edge cases in concerto packages.
Related Issues
Author Checklist
--signoff
option of git commit.main
fromfork:branchname