Skip to content

Extract modelType out of ModelElement #2583

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

Merged
merged 24 commits into from
Mar 18, 2021

Conversation

jcollins-g
Copy link
Contributor

This breaks up one big chunk of those if type/do this bits in ModelElement that need to be eliminated. In this case, it makes generic type aliases easier to implement and think about with this done.

@google-cla google-cla bot added the cla: yes Google CLA check succeeded. label Mar 17, 2021
@jcollins-g jcollins-g requested a review from srawlins March 17, 2021 23:45
@coveralls
Copy link

coveralls commented Mar 17, 2021

Coverage Status

Coverage decreased (-0.02%) to 91.662% when pulling a0fc976 on jcollins-g:modelType-refactor into d0a87c7 on dart-lang:master.

Copy link
Member

@srawlins srawlins left a comment

Choose a reason for hiding this comment

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

LGTM. Very nice refactor and simplification

if (_modelType == null) {
if (originalMember != null) {
_modelType =
ElementType.from(originalMember.type, library, packageGraph);
Copy link
Member

Choose a reason for hiding this comment

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

Would originalMember ?? element work here?

_modelType =
ElementType.from((originalMember ?? element).type, library, packageGraph);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this can all be collapsed. Done here and elsewhere

@jcollins-g jcollins-g merged commit 7022b7f into dart-lang:master Mar 18, 2021
@jcollins-g jcollins-g deleted the modelType-refactor branch March 18, 2021 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants