move from telescope/path to plenary/path

[anott03]

Closes #387

[anott03]

Some of the changes in the diffs don't make sense... it's showing a bunch of changes regarding the names actions that shouldn't be there. When comparing my code to what I see on master by hand it seems to match. Can someone take a look?

[Conni2461]

You probably based this branch on your last branch rather than on master. You could try to rebase it on current master. Not sure if it will work tho 😆

Note: I will look at your plenary work first. I probably won't look at this PR today :)

[anott03]

@Conni2461 I'll try rebasing - thanks!

Note: I will look at your plenary work first. I probably won't look at this PR today :)

Of course. We all have things we need to do :)

[anott03]

This needs to be worked on some more after changes being made to plenary

[Conni2461]

@anott03 Could you check if this one will be fixed when using path:new #394

Also did you have any luck with rebasing?

[anott03]

@Conni2461 sure I'll take a look at #394 today.

As for the rebasing, I took a step back from telescope and was trying to get everything done with plenary first (which I think I have, so I'll get back to this today or tomorrow). And while I'm at it, I'll switch all instances of vim.fn.getcwd to vim.loop.cwd.

[anott03]

@Conni2461 I just rebased against master, and it seems to have worked (all tests pass, it seems to be running fine, etc.). As for #394, I tested it in the following way:

1. Checkout telescope master
2. cd ~/.config/nvim (which is a symlink to ~/repos/dotfiles/nvim
3. run :Telescope find_files

I then repeated the above steps on my branch, plenaryPath. The results I yielded were the same for both branches, and that is that it successfully listed the files I had in that directory. I'm not sure if that means the issue has been solved, or if I need to take other steps to reproduce it. I'll also perform these tests on a symlink that doesn't point to a git repo and see if that makes a difference.

[tjdevries]

(What do you need me to do 😆 )

[anott03]

@tjdevries Unless there are changes that need to be made to this code before it can be merged (stuff to resolve the failed linting test maybe?) I'd say there isn't much to do regarding this issue until the PR to plenary has been merged since this is dependent on that. It's probably worth it to see if someone can recreate #394. I've tried and haven't been able to.

[anott03]

@Conni2461 I don't think we should wait on Path:escape before merging this... I think it makes more sense just to resolve the conflicts and merge this as is and then create a separate PR for escaping paths if necessary.

[Conni2461]

Works for me. I am currently really busy (sorry exams and stuff) so i won't be around that much but i 100% trust @elianiva if he wants to finish it with you :)

[anott03]

No worries - good luck with the exams! I'll talk to @elianiva here or on gitter when he's free :)

[elianiva]

probably better talk it here, Gitter is a bit janky for me. It deletes DM from other people randomly for some reason 😆

[tjdevries]

Do you need anything from me?

[anott03]

@tjdevries I believe this is good to go - it just needs to be reviewed by someone. I had talked with Conni about waiting until we have a Path:escape in plenary but I think we should just merge this as is and then open a new PR if/when an escape function comes around.

[anott03]

@elianiva Can you take a look at the second conflict in actions/init.lua? That's the only one I'm not quite sure how to resolve yet...

[elianiva]

remove this part from your branch and edit this file accordingly, specifically this line and this line

We've moved the action that was previously in actions/init.lua to actions/set.lua. Hope it helps :)

[anott03]

Makes sense - thanks :)

[anott03]

I ended up resetting and doing everything over again because it turned out to be easier than rebasing against the latest changes (at least for me). In the processing of doing everything I realized that not only did telescope.path have some path related functions, but so did utils, so this now gets rid of any path functions in utils in favor of plenary as well.

@Conni2461 and @elianiva can one of you check this out?

[Conni2461]

Also we wanna remove telescope.path so we should replace each function with a error message that says. "Please use plenary.path" or similar. So extensions know that they have to move to the new way and then remove the whole module in a month or so.

Similar to

telescope.nvim/lua/telescope/utils.lua

Lines 420 to 434 in 49b86b4

	utils.strdisplaywidth = function()
	error("strdisplaywidth deprecated. please use plenary.strings.strdisplaywidth")
	end
	utils.utf_ptr2len = function()
	error("utf_ptr2len deprecated. please use plenary.strings.utf_ptr2len")
	end
	utils.strcharpart = function()
	error("strcharpart deprecated. please use plenary.strings.strcharpart")
	end
	utils.align_str = function()
	error("align_str deprecated. please use plenary.strings.align_str")
	end

for example (i think we could remove those functions now btw)

lets finish this pr in the next two days. I am here if you need me :)

[anott03]

@Conni2461 how do we typically handle deprecating things like path.separator which are variables rather than functions?

[anott03]

Could we do something like this?

local path = setmetatable({}, {
  __index = function()
    error("telescope.path is deprecated. please use plenary.path instead")
  end
})

return path

[Conni2461]

Yes thats better for our case because we deprecate a whole module, not just a couple of functions 👍

[Conni2461]

LGTM

I need to test it, so i will run it today and see if something doesnt work. If all works out i'll merge this evening

Thank you very much :) and thank you for your patience :)

[Conni2461]

I talked with tj for a second. He wants to keep the module for now but throw an error. Just so extensions have a week or so to switch over to plenary.path

I am going to merge this now.

Thanks for all your work and your patience with me :)

[clason]

Shouldn't this get an entry in https://github.com/nvim-telescope/telescope.nvim/blob/master/doc/telescope_changelog.txt?

[Conni2461]

I forgot about this. Calling the function will yell at you tho 😆

[clason]

Yeah, but some explanation -- and migration guide! -- would be good since it's not as simple as replacing require'telescope.path' with require'plenary.path' ;)

[Conni2461]

I'll add one i noticed something else super small too too, i shouldn't have merged. Honest mistake. I was just glad right now that it was done and it didn't break anything inside telescope 😆

[anott03]

@Conni2461 thanks for all your help! Lmk if there's anything else you'd like me to do :)

And sorry again for disappearing for weeks on end!