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

Use repo assignable / mentionable users in finder #594

Merged
merged 9 commits into from
Sep 13, 2024

Conversation

wd60622
Copy link
Collaborator

@wd60622 wd60622 commented Sep 8, 2024

Describe what this PR does / why we need it

Use repo's assignable / mentionable users to populate finder instead of general GitHub search.

TODO:

  • Add a configuration option so that the previous behavior can be used if desired
  • Implement for the other pickers / refactor user list logic to separate file. Thoughts on this @pwntester?
  • Update any documentation

Does this pull request fix one issue?

Closes #423

Describe how you did it

Pull out the finder creation logic

Describe how to verify it

Use Octo assignee add or Octo reviewer add to see the new behavior.

Special notes for reviews

@wd60622 wd60622 changed the title Use repo mentionable users in finder Use repo assignable / mentionable users in finder Sep 8, 2024
@wd60622 wd60622 marked this pull request as ready for review September 8, 2024 18:14
Copy link
Owner

@pwntester pwntester left a comment

Choose a reason for hiding this comment

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

It would be great to move the shared code to a shared location so other pickers can reuse it. But since there is no other implementation for fzf-lua yet, Im ok if you want to merge this and let the implementator for fzf-lua to refactor that code.

@pwntester
Copy link
Owner

@milogert just a heads up about a new implementation of the picker for choosing users in case you can port it to fzf-lua

@wd60622
Copy link
Collaborator Author

wd60622 commented Sep 10, 2024

Might be good to add to the readme before merge if you think this config change is reasonable @pwntester

@pwntester
Copy link
Owner

Yep, please update the docs so users know the three options and the differences between them

@wd60622
Copy link
Collaborator Author

wd60622 commented Sep 10, 2024

I've added to the README. Let me know what you think about the location @pwntester

@pwntester
Copy link
Owner

I've added to the README. Let me know what you think about the location @pwntester

That should be enough, thanks!

@GrimalDev
Copy link

Tested the integration on my config. You guys rule! It is a banger and the documentation is crystal clear.

@pwntester pwntester merged commit ba5f510 into pwntester:master Sep 13, 2024
2 checks passed
@pwntester
Copy link
Owner

Thanks @wd60622!

@milogert
Copy link
Contributor

@milogert just a heads up about a new implementation of the picker for choosing users in case you can port it to fzf-lua

Thanks! I can start taking a look at this 👌

@wd60622 wd60622 deleted the mentionable-users branch September 19, 2024 23:21
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.

Filter assignees to current repo
4 participants