search

danielfalk / smart-open.nvim

Neovim plugin for fast file-finding
MIT License
498 stars 25 forks source link

feat: filename_first option #37

Closed YodaEmbedding closed 1 year ago

danielfalk commented 1 year ago

The code looks right, but I don't have time to test this yet. I'm particularly concerned with the highlighting. Does it look OK in all circumstances?

YodaEmbedding commented 1 year ago

When filename_first = false, the highlighting is 1 character too long:

YodaEmbedding commented 1 year ago

Fixed in https://github.com/danielfalk/smart-open.nvim/pull/37/commits/733b8e339490cf00b63ab74fe244f593fc084486. Note that the filename_first = true was also 1 character too long, though it wasn't as noticeable.

YodaEmbedding commented 1 year ago

The search highlighting for filename_first = false isn't working properly for directories, though.

Correct (except for tests/utils.py):

Incorrect:

The first word is incorrectly highlighted. I wonder if that's because the highlighting logic was expecting a string in the format filename path/to/dir... Actually, yeah, look: scripTS, resulTS.py, comprESsai_trainer/... all occur on the fifth character, so two of them are linked in some way.

YodaEmbedding commented 1 year ago

Fixed in https://github.com/danielfalk/smart-open.nvim/pull/37/commits/a961ee00c6447d813ba935481d22345f38dc1177:

danielfalk commented 1 year ago

Good work first of all. I was able to try it out. There is just one remaining thing I noticed. With filename_first = true, there is no leading slash for files in the base directory, but for filename_first = false, I'm seeing that: image

(should just be stylua.toml, not /stylua.toml)

YodaEmbedding commented 1 year ago

Fixed in https://github.com/danielfalk/smart-open.nvim/pull/37/commits/92e523a228524f40b9b2414efc27b2cce4fcef6c.

YodaEmbedding commented 1 year ago

I think that the path formatters may need more tweaking. For instance, given the directory tree:

~/cwd/
     ├── cwd
     │   ├── init.lua
     │   └── not_an_init.txt
     ├── init.lua
     └── not_an_init.txt

And if cwd = ~/cwd, then we get:

Notice that ~/cwd/init.lua is formatted a bit unusually in both cases and may be confused with ~/cwd/cwd/init.lua. Perhaps better is:

cwd/init.lua    (for ~/cwd/cwd/init.lua)
init.lua        (for ~/cwd/init.lua)

But that's probably outside the scope of this PR.

danielfalk commented 1 year ago

Looks good. Thanks!