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

fix(*): expose model util methods, remove redundant type lookup #469

Merged
merged 1 commit into from
Jul 28, 2022

Conversation

sstone1
Copy link
Contributor

@sstone1 sstone1 commented Jul 27, 2022

Changes

  • Expose certain ModelUtil methods and correct the TypeScript types for parseNamespace
  • Remove a seemingly redundant codepath from ModelFile.getFullyQualifiedTypeName

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to master from fork:branchname

@sstone1 sstone1 self-assigned this Jul 27, 2022
@@ -393,13 +393,7 @@ class ModelFile {
}
}
else {
// check whether type is defined in another file
const fqn = this.resolveImport(type);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, we get the FQN of the type. For import { Thing } from org.example and import org.example.Thing this is simple, as all the information to identify that the FQN of Thing is org.example.Thing.

For wildcard imports import org.example.* (which we want to drop), we have to visit the model manager and look through all wildcard imports for the namespace that contains the type we want.

Either way, we end up with the FQN of the type or we throw an error as the type cannot be resolved.

Comment on lines -398 to -402
const modelFile = this.getModelManager().getModelFile(ModelUtil.getNamespace(fqn));
if (!modelFile) {
return null;
}
return modelFile.getLocalType(fqn).getFullyQualifiedName();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of this code is seemingly redundant - we use the FQN that we already have to locate the model file of the type, then we get the type, and then we get the FQN of the type (which we already know!) and return that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe worth adding the motivation for this actually - yes, it will have a small performance benefit, but it actually enables you to get fully qualified names out of a ModelFile instance without having to have the imported models present in your ModelManager now that we have versioned imports and are looking at dropping wildcard imports.

@sstone1 sstone1 requested review from dselman and mttrbrts July 27, 2022 13:02
@mttrbrts mttrbrts merged commit 9576cbd into accordproject:master Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants