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

Add option to search through session panes #24

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

paluh
Copy link

@paluh paluh commented Apr 3, 2022

Great plugin! Thanks a lot for writing it!

I'm not sure if you would like to merge this addition - it is a breaking change (I've changed all_panes boolean option to panes which is a choice) which I have not discussed ;-)

@andersevenrud andersevenrud self-requested a review April 3, 2022 19:11
@andersevenrud
Copy link
Owner

Thanks for using my plugin and for the contribution!

I really like the idea of this, but I think maybe this could be introduced without any immediate breaking changes and instead provide a migration path.

So maybe keep the all_panes option, but emit a warning if it's been set to true and suggest using the option introduced here ?

Copy link
Owner

@andersevenrud andersevenrud left a comment

Choose a reason for hiding this comment

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

Linter seems to have tripped :)

@paluh
Copy link
Author

paluh commented Apr 3, 2022

Linter seems to have tripped :)

Oh, I'm really sorry about that. I have to setup lua linting and fix that for sure.

So maybe keep the all_panes option, but emit a warning if it's been set to true and suggest using the option introduced here ?

We can try to use both options but I'm not sure if this won't be more confusing (the interaction between different setups etc.)?

Or we can tag the current version and prepare next proper release? Then we can bump the minor version with appropriate change log and provide migration error messages?

We can add to the release some additional changes:

  • I've already made trigger_characters_* optional (I'm using keyword_length myself)
  • I'm thinking that limiting the result number or the actual search space per pane could be another useful addition - what do you think?

P.S.
Unfortunately I'm not able to to hack on these fixes probably during the week and I will do them during next weekend I think ;-)

@andersevenrud
Copy link
Owner

Oh, I'm really sorry about that. I have to setup lua linting and fix that for sure.

No worries! I probably should add a contribution guide here as there's linting on the commit messages as well which will fail after the code linting. This repo uses https://www.conventionalcommits.org/en/v1.0.0/

We can try to use both options but I'm not sure if this won't be more confusing (the interaction between different setups etc.)?

I feel like if the documentation of all_panes was removed and if enabled was made to emit a warning message, that would suffice. If everyone used a package manager that warned about changes like this by scanning the commit messages I would just go strait for it, but sadly that's not the case, hehe. I might be overthinking this as well 🤷‍♂️

I've already made trigger_characters_* optional (I'm using keyword_length myself)

I'll have to look into that.

I'm thinking that limiting the result number or the actual search space per pane could be another useful addition - what do you think?

What would be the benefit of this ?

Unfortunately I'm not able to to hack on these fixes probably during the week and I will do them during next weekend I think

Take your time!

@andersevenrud andersevenrud self-assigned this Apr 10, 2022
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.

None yet

2 participants