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

Meta: Use new APIs in update-pr-from-base-branch #6103

Merged
merged 20 commits into from Nov 1, 2022
Merged

Meta: Use new APIs in update-pr-from-base-branch #6103

merged 20 commits into from Nov 1, 2022

Conversation

yakov116
Copy link
Member

@yakov116 yakov116 commented Oct 23, 2022

Test URLs

refined-github/sandbox#9 (Conflict)
refined-github/sandbox#11

Screenshot

NA

@yakov116 yakov116 added the meta Related to Refined GitHub itself label Oct 23, 2022
@yakov116 yakov116 changed the title Use new APIs in update-pr-from-base-branch Meta: Use new APIs in update-pr-from-base-branch Oct 23, 2022
@yakov116
Copy link
Member Author

@busches will this affect GHE?

@yakov116 yakov116 marked this pull request as ready for review October 23, 2022 20:38
@busches
Copy link
Member

busches commented Oct 23, 2022

@yakov116 you can check the GHE docs to see what, if any, versions support it yet. Guessing it's not in any version yet.

@fregante
Copy link
Member

@yakov116 we have probably never waited for GHE to use updated APIs

@yakov116
Copy link
Member Author

yakov116 commented Oct 23, 2022

This will break the entire feature, since the api call will fail.

I don't remember in the past ever having a case where an API was not available on GHE.

However I do remember holding off on v4 until GHE had normal support.

I am not waiting to support the API I am just using a fallback.

@busches
Copy link
Member

busches commented Oct 23, 2022

I do use this feature all the time, so I appreciate the thought. 😃

source/features/restore-file.tsx Outdated Show resolved Hide resolved
source/features/update-pr-from-base-branch.tsx Outdated Show resolved Hide resolved
};
comparison: {
status: 'BEHIND' | 'DIVERGED' | 'AHEAD' | 'IDENTICAL';
};
};
Copy link
Member

Choose a reason for hiding this comment

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

You can merge these into flat object, no need to have prInfo and comparison keys. While we're at it it can be simplified further, like setting diverged: true instead of matching strings outside

Copy link
Member Author

Choose a reason for hiding this comment

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

While we're at it it can be simplified further, like setting diverged: true instead of matching strings outside

Can you explain this one more time I am not sure I understood what you said

Comment on lines +50 to 54
if (repository.pullRequest.headRef.compare.status !== 'DIVERGED') {
return;
}

return repository.pullRequest;
Copy link
Member

Choose a reason for hiding this comment

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

This just mixes the logic of a specific feature with this generic get pr info function, which could return the PR info regardless of the need to update the PR.

I suggested returning diverged: true for both API calls, i.e. something like

Suggested change
if (repository.pullRequest.headRef.compare.status !== 'DIVERGED') {
return;
}
return repository.pullRequest;
return {
...repository.pullRequest,
diverged: repository.pullRequest.headRef.compare.status === 'DIVERGED'
};

And the logic just checks pr.diverged, or better yet pr.needsUpdate as a combination of mergeable and diverged


I suppose it's ok as is for now if it's tested, but it'll need to be changed eventually. 👍

@yakov116 yakov116 merged commit 987b566 into main Nov 1, 2022
@yakov116 yakov116 deleted the v4 branch November 1, 2022 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Related to Refined GitHub itself
Development

Successfully merging this pull request may close these issues.

None yet

3 participants