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.

Sign up for GitHub

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

Jump to bottom

fix: Fixes MR selection if there are multiple targets #357

Merged
merged 5 commits into from
Sep 7, 2024
Merged

fix: Fixes MR selection if there are multiple targets #357

merged 5 commits into from
Sep 7, 2024

Conversation

harrisoncramer
Copy link
Owner

@harrisoncramer harrisoncramer commented Sep 6, 2024 •
edited
Loading

Addresses #319

harrisoncramer and others added 3 commits September 5, 2024 14:00
@harrisoncramer
Popup and Keymap Updates (#356)
6500ef1 
feat: Enable always jumping to discussion tree (#352)
feat: Enables motions for easier range selection when creating comments/suggestions (e.g. s3j, c3j) (#353)
fix: Makes help popup not editable and close it on BufLeave (#355)

This is a #MINOR release
@harrisoncramer
Merge branch 'main' into develop
76b3843
@harrisoncramer
added fix
f27f538
@harrisoncramer harrisoncramer linked an issue Sep 6, 2024 that may be closed by this pull request
choose_merge_request does not select correct MR if there are several MRs into different target branches #319
Closed
3 tasks
@harrisoncramer harrisoncramer changed the base branch from main to develop September 6, 2024 01:19
@harrisoncramer
Copy link
Owner Author

harrisoncramer commented Sep 6, 2024 •
edited
Loading

Hey @jakubbortlik I've fixed that issue.

If you are just opening the reviewer in a merge request and there are multiple options, I'll throw an error in the Go server and tell you to choose one via the choose_merge_request option. The choose_merge_request function now sets a value in the shared state that specifies the target for the MR selector, which is passed along to the server.

This should resolve your issue, can you please give it a try?

-- Also heads up this has Go changes, so you'll have to rebuild the server

@harrisoncramer harrisoncramer changed the title fix: Fixes MR selection if there are multiple for one branch fix: Fixes MR selection if there are multiple targets Sep 6, 2024
@harrisoncramer harrisoncramer marked this pull request as ready for review September 6, 2024 01:24
@jakubbortlik
Copy link
Collaborator

Hi Harrison, I confirm that in this branch the issue is solved. Thanks!

@jakubbortlik
Copy link
Collaborator

Maybe you could show the error and run choose_merge_request right away. I believe that's what most users will want to do anyway.

@harrisoncramer
Copy link
Owner Author

harrisoncramer commented Sep 7, 2024 •
edited
Loading

Yeah, I thought about having that, but it would add some additional complexity to the codebase for an edge case that is very rarely going to occur, so I don't think it's worth it 👍

Erroring on an endpoint designed to fetch one specific MR is more idiomatic than returning a 200 and requiring the client to do additional work and re-call it

@harrisoncramer harrisoncramer merged commit a02de36 into develop Sep 7, 2024
6 checks passed
@harrisoncramer harrisoncramer mentioned this pull request Sep 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakubbortlik jakubbortlik Awaiting requested review from jakubbortlik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

choose_merge_request does not select correct MR if there are several MRs into different target branches
2 participants
@harrisoncramer @jakubbortlik