Skip to content

[completion] Clarify and simplify not-recommended state #36961

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 2 commits into from
Apr 19, 2021

Conversation

benlangmuir
Copy link
Contributor

Combine IsNotRecommended with NotRecommendedReason and improve the names
of the existing cases to more clearly identify the cases they apply to.
Now all not-recommended completions are required to have a reason.

Combine IsNotRecommended with NotRecommendedReason and improve the names
of the existing cases to more clearly identify the cases they apply to.
Now all not-recommended completions are required to have a reason.
@benlangmuir benlangmuir requested a review from rintaro April 19, 2021 18:43
@benlangmuir
Copy link
Contributor Author

@swift-ci please smoke test

bool isNotRecommended() const {
return NotRecommended;
}
bool isNotRecommended() const { return NotRecommended != None; }
Copy link
Member

Choose a reason for hiding this comment

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

Please

Suggested change
bool isNotRecommended() const { return NotRecommended != None; }
bool isNotRecommended() const { return NotRecommended != NotRecommendedReason::None; }

None sounds too generic for a symbol that can be referenced as a direct member of CodeCompletionResult (i.e. CodeCompletionResult::None). We probably should make NotRecommendedReason (and ExpectedTypeRelation, etc) a scoped enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM, I'll make it enum class NotRecommendedReason. I'll leave ExpectedTypeRelation for follow-up.

@benlangmuir
Copy link
Contributor Author

@swift-ci please smoke test

@benlangmuir benlangmuir merged commit 89039a8 into swiftlang:main Apr 19, 2021
@benlangmuir benlangmuir deleted the not-recommended-improvement branch April 19, 2021 22:48
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