axkirillov / hbac.nvim

Heuristic buffer auto-close
MIT License
197 stars 10 forks source link

feat(telescope)!: create telescope extension for pin picker #17

Closed Nimjii closed 10 months ago

Nimjii commented 10 months ago

Hey there! This PR creates a telescope extension that replaces the telescope command of this plugin. Technically this is a BREAKING CHANGE although things still work mostly the same.

Changes:

The big advantage of using a telescope extension is that hbac doesn't need to load telescope as a dependency. Instead it's the other way around: if you use the telescope picker, telescope will have to load hbac as a dependency (although chances are it will already be loaded anyway).

I personally lazyload telescope, so as long as I don't use it, it won't be loaded but previously hbac always loaded telscope no matter what, which rendered my lazyloading setup kind of useless. This fixes that problem.

I also added a new setting, which lets you avoid the default telescope mappings if you don't want them.

I updated the docs accordingly, but feel free to notify me if I missed anything. All previous features (passing options to the picker directly, etc.) should still work the same.

axkirillov commented 10 months ago

Thanks you for the contribution! This looks pretty good, but I want to get @al-ce opinion on this.

al-ce commented 10 months ago

Hey, thanks for the PR @Nimjii ! I made a commit on a branch last night to address the lazy loading issue but I wanted to sleep on it before I made a PR https://github.com/al-ce/hbac.nvim/commit/824c1eb9c3bc160e31f2161c4d0304113e262511

I do like your reasoning regarding trading the dependencies. It's more in line with how other plugins that use Telescope would lazyload it. The option to disable the default mappings is very nice too. Overall I think this is a good change.

My only consideration is about removing the Hbac telescope command without a deprecation plan. What would you all think about leaving it in, maybe make a simple pcall for Telescope in the subcommands.telescope function with an early return, and make a deprecation notice in the commits and the README? The README changes in this PR could remain as is otherwise, silently removing mentions of :Hbac telescope. Not too attached to this though if you're both ok with removing the command, (and I just made a PR with a breaking change last month). I also hope people know not to run Sync on their package manager without checking the commit messages.

I'll pass the buck to @axkirillov on that decision, and I have one other review comment I'm about to make, but other than that this has my thumbsup. Thanks again!

axkirillov commented 10 months ago

I am OK with breaking changes. As an extra way to notify users, how about when:Hbac telescope is called we just call vim.notify with a deprecation message? Like "the telescope command has been moved to a telescope extension, check README"?

al-ce commented 10 months ago

"the telescope command has been moved to a telescope extension, check README"?

Good idea, could we also add the new command to that notification? "use :Telescope hbac buffers instead"

al-ce commented 10 months ago

Everything works well on my end. Commands, telescope actions, old options, new options, migration message, and lazy loading of telescope/plenary/nvim-web-devicons

axkirillov commented 10 months ago

@Nimjii Can you merge it into the develop branch? I'll test it out and prepare the release then.

Nimjii commented 10 months ago

I'm pretty sure that I'm not allowed to merge but I might also just be stupid. ^^

axkirillov commented 10 months ago

@Nimjii you are definitely not stupid! I think we are all still the process of figuring this stuff out 😅