Skip to content

Add option to search through session panes - #24

Open
paluh wants to merge 2 commits into
andersevenrud:mainfrom
paluh:main
Open

Add option to search through session panes#24
paluh wants to merge 2 commits into
andersevenrud:mainfrom
paluh:main

Conversation

@paluh

@paluh paluh commented Apr 3, 2022

Copy link
Copy Markdown

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
Copy Markdown
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 ?

@andersevenrud andersevenrud left a comment

Copy link
Copy Markdown
Owner

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

paluh commented Apr 3, 2022

Copy link
Copy Markdown
Author

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
Copy Markdown
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.

2 participants