Skip to content

Conversation

@Yuuki77
Copy link
Contributor

@Yuuki77 Yuuki77 commented Oct 29, 2021

I took over #1406 (comment)
I could not update the original pr.
If there is a way, please let me know!

since It confliced a lot for now, it only shows the pr number associated with branch.

@Yuuki77 Yuuki77 force-pushed the gh-integration branch 4 times, most recently from c40d3f2 to 7d3213d Compare October 30, 2021 02:00
@Yuuki77
Copy link
Contributor Author

Yuuki77 commented Oct 30, 2021

hmm integration tests on ubuntu failed.
I have been investigating.
@jesseduffield do you have any idea? 🙏

@jesseduffield
Copy link
Owner

@Yuuki77 those failing integration tests relate to pushing/pulling from an origin. I would do some pushing/pulling locally and see if anything weird happens. You can also run an integration test locally with go run test/lazyintegration/main.go (it opens a little gui for selecting the test. If you press enter you'll see it run)

@Yuuki77
Copy link
Contributor Author

Yuuki77 commented Oct 31, 2021

thanks! I will test it as well!
let me try if I can fix it

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

I know this is a WIP but I left a bit of feedback :)

@Yuuki77 Yuuki77 force-pushed the gh-integration branch 3 times, most recently from 94a2ac9 to 6c679f2 Compare October 31, 2021 06:38
@Yuuki77
Copy link
Contributor Author

Yuuki77 commented Oct 31, 2021

@jesseduffield Thank you for the quick reviews!
I have updated the pr!
Could you take a look when you have time!

@Yuuki77 Yuuki77 changed the title WIP Continuations of Gh integration Continuations of Gh integration Oct 31, 2021
Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

few more things :)

@Yuuki77
Copy link
Contributor Author

Yuuki77 commented Feb 25, 2022

@jesseduffield
It has been some time since my last revision, but I was able to find the time.
I fixed the conflicts, can you review it when you have time? 😄

@Yuuki77 Yuuki77 force-pushed the gh-integration branch 7 times, most recently from 9086c3e to f9795b9 Compare August 16, 2022 12:44
@Yuuki77
Copy link
Contributor Author

Yuuki77 commented Aug 16, 2022

@jesseduffield I fixed the conflict! I want to get this pr merged.
Let me know if you have any comments!

@jesseduffield
Copy link
Owner

@Yuuki77 testing this on lazygit it's not quite formatting properly:
image

@Yuuki77
Copy link
Contributor Author

Yuuki77 commented Aug 19, 2022

@jesseduffield Thank you for the quick reviews!
I updated it and could you try it again?

@jesseduffield
Copy link
Owner

I've tested again. If I switch repos from A to B, I don't see B's pull request numbers

@Yuuki77
Copy link
Contributor Author

Yuuki77 commented Aug 19, 2022

@jesseduffield that issue should be fixed.

@Yuuki77 Yuuki77 closed this Sep 22, 2022
@jesseduffield
Copy link
Owner

Sorry @Yuuki77 ! I never found the time to properly review this, my bad. I've got some changes locally that I can push:

  • more specific error message when gh version command fails (likewise with the git version command)
  • moved code from helpers.host_helper to git_commands.hosting_service
  • using struct for PR key
  • moving PR refresh code into refresh.go, changing logic to work more like reflog commits
  • some copy updates

Something we'll want to do before merging this PR is moving the logic from GetRepoInfoFromURL into pkg/commands/hosting_service/hosting_service.go so that we're containing all of our hosting service specific code in the one place (and it will make it easier to support PR numbers for other hosting services down the line).

There's a couple more things that would be good (not necessary for merging the PR):

  • support for branches whose UpstreamRemote is of the form 'git@.../lazygit.go'. These don't actually have a remote to match on locally, but they do contain the owner name in the URL
  • structuring the code to support other PR structures e.g. not every hosting service has an owner/branch structure: azure has org/project/branch.

@Yuuki77 if you want to step away from this feature that's understandable, I'm happy to pick it up. Thanks for taking the time to work on the feature at any rate :)

@Yuuki77
Copy link
Contributor Author

Yuuki77 commented Sep 25, 2022

@jesseduffield no worries 😄
This is a bigger change than I originally thought, and it would be better to have this feature taken over by someone.
This feature is very useful and I hope it will be merged into master soon!
I really like lazygit, so I will continue to implement other features when I have time. 👍

@jesseduffield
Copy link
Owner

Good to hear @Yuuki77

@jesseduffield jesseduffield mentioned this pull request Sep 25, 2022
6 tasks
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.

3 participants