-
Notifications
You must be signed in to change notification settings - Fork 631
Remove uses of property existence with in
#6932
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -113,11 +113,13 @@ export interface Account extends Actor { | |||||||||
| } | ||||||||||
|
|
||||||||||
| export function isAccount(x: Actor | Team | Node | undefined | null): x is Account { | ||||||||||
| return !!x && 'name' in x && 'email' in x; | ||||||||||
| const asAccount = x as Partial<Account>; | ||||||||||
| return !!asAccount && !!asAccount?.name && !!asAccount?.email; | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic for checking if an object is an Account is incorrect. It should check if the properties exist (are not undefined), not if they are truthy. An account could have name: '' or email: '', which would make this function return false incorrectly.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic requires both name and email to be truthy, but according to the Account interface, email is optional (email?: string). This will incorrectly return false for valid Account objects that don't have an email.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||
| } | ||||||||||
|
|
||||||||||
| export function isTeam(x: Actor | Team | Node | undefined | null): x is Team { | ||||||||||
| return !!x && 'slug' in x; | ||||||||||
| const asTeam = x as Partial<Team>; | ||||||||||
| return !!asTeam && !!asTeam?.slug; | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic for checking if an object is a Team is incorrect. It should check if the slug property exists (is not undefined), not if it is truthy. A team could have slug: '', which would make this function return false incorrectly.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||
| } | ||||||||||
|
|
||||||||||
| export interface Team { | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -105,26 +105,28 @@ export interface MergeQueueEntry { | |||||||||
|
|
||||||||||
| export function reviewerId(reviewer: ITeam | IAccount): string { | ||||||||||
| // We can literally get different login values for copilot depending on where it's coming from (already assignee vs suggested assingee) | ||||||||||
| return isTeam(reviewer) ? reviewer.id : (reviewer.specialDisplayName ?? reviewer.login); | ||||||||||
| return isITeam(reviewer) ? reviewer.id : (reviewer.specialDisplayName ?? reviewer.login); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export function reviewerLabel(reviewer: ITeam | IAccount | IActor | any): string { | ||||||||||
| return isTeam(reviewer) ? (reviewer.name ?? reviewer.slug ?? reviewer.id) : (reviewer.specialDisplayName ?? reviewer.login); | ||||||||||
| return isITeam(reviewer) ? (reviewer.name ?? reviewer.slug ?? reviewer.id) : (reviewer.specialDisplayName ?? reviewer.login); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export function isTeam(reviewer: ITeam | IAccount | IActor | any): reviewer is ITeam { | ||||||||||
| return 'org' in reviewer; | ||||||||||
| export function isITeam(reviewer: ITeam | IAccount | IActor | any): reviewer is ITeam { | ||||||||||
| const asITeam = reviewer as Partial<ITeam>; | ||||||||||
| return !!asITeam.org; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export interface ISuggestedReviewer extends IAccount { | ||||||||||
| isAuthor: boolean; | ||||||||||
| isCommenter: boolean; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export function isSuggestedReviewer( | ||||||||||
| export function isISuggestedReviewer( | ||||||||||
| reviewer: IAccount | ISuggestedReviewer | ITeam | ||||||||||
| ): reviewer is ISuggestedReviewer { | ||||||||||
| return 'isAuthor' in reviewer && 'isCommenter' in reviewer; | ||||||||||
| const asISuggestedReviewer = reviewer as Partial<ISuggestedReviewer>; | ||||||||||
| return !!asISuggestedReviewer.isAuthor && !!asISuggestedReviewer.isCommenter; | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic for checking if a reviewer is an ISuggestedReviewer is incorrect. It should check if both properties exist (are not undefined), not if they are truthy. A reviewer could have isAuthor: false and isCommenter: false, which would make this function return false incorrectly.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic for checking if an object is an ISuggestedReviewer is incorrect. The function should return true when both properties exist (regardless of their boolean values), but the current implementation requires both to be truthy. This could cause false negatives when isAuthor or isCommenter are explicitly false.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||
| } | ||||||||||
|
|
||||||||||
| export interface IProject { | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -361,8 +361,8 @@ export function convertRESTPullRequestToRawPullRequest( | |
| }; | ||
|
|
||
| // mergeable is not included in the list response, will need to fetch later | ||
| if ('mergeable' in pullRequest) { | ||
| item.mergeable = pullRequest.mergeable | ||
| if ((pullRequest as OctokitCommon.PullsGetResponseData).mergeable !== undefined) { | ||
| item.mergeable = (pullRequest as OctokitCommon.PullsGetResponseData).mergeable | ||
| ? PullRequestMergeability.Mergeable | ||
| : PullRequestMergeability.NotMergeable; | ||
| } | ||
|
|
@@ -600,14 +600,39 @@ export interface RestAccount { | |
| type: string; | ||
| } | ||
|
|
||
| export interface GraphQLAccount { | ||
| login: string; | ||
| url: string; | ||
| avatarUrl: string; | ||
| email?: string; | ||
| id: string; | ||
| name?: string; | ||
| __typename: string; | ||
| } | ||
|
|
||
| export function parseAccount( | ||
| author: { login: string; url: string; avatarUrl: string; email?: string, id: string, name?: string, __typename: string } | RestAccount | null, | ||
| author: GraphQLAccount | RestAccount | null, | ||
| githubRepository?: GitHubRepository, | ||
| ): IAccount { | ||
| if (author) { | ||
| const avatarUrl = 'avatarUrl' in author ? author.avatarUrl : author.avatar_url; | ||
| const id = 'node_id' in author ? author.node_id : author.id; | ||
| const url = 'html_url' in author ? author.html_url : author.url; | ||
| let avatarUrl: string; | ||
| let id: string; | ||
| let url: string; | ||
| let accountType: string; | ||
| if ((author as RestAccount).html_url) { | ||
| const asRestAccount = author as RestAccount; | ||
| avatarUrl = asRestAccount.avatar_url; | ||
| id = asRestAccount.node_id; | ||
| url = asRestAccount.html_url; | ||
| accountType = asRestAccount.type; | ||
| } else { | ||
| const asGraphQLAccount = author as GraphQLAccount; | ||
| avatarUrl = asGraphQLAccount.avatarUrl; | ||
| id = asGraphQLAccount.id; | ||
| url = asGraphQLAccount.url; | ||
| accountType = asGraphQLAccount.__typename; | ||
|
Comment on lines
617
to
+633
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using property existence to determine the type is not reliable and contradicts the PR's goal of removing 'in' operator usage. Consider using a more explicit type discriminator or a proper type guard function. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||
| } | ||
|
|
||
| // In some places, Copilot comes in as a user, and in others as a bot | ||
| return { | ||
| login: author.login, | ||
|
|
@@ -617,7 +642,7 @@ export function parseAccount( | |
| id, | ||
| name: author.name ?? COPILOT_ACCOUNTS[author.login]?.name ?? undefined, | ||
| specialDisplayName: COPILOT_ACCOUNTS[author.login] ? (author.name ?? COPILOT_ACCOUNTS[author.login].name) : undefined, | ||
| accountType: toAccountType('__typename' in author ? author.__typename : author.type), | ||
| accountType: toAccountType(accountType), | ||
| }; | ||
| } else { | ||
| return { | ||
|
|
||
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.
Checking for toString method existence is similar to the 'in' operator usage that this PR aims to eliminate. Consider using a more explicit type checking approach.
Copilot uses AI. Check for mistakes.