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

Remove dependency on sqlite #91

Closed
gegoune opened this issue Feb 19, 2023 · 6 comments · Fixed by #148
Closed

Remove dependency on sqlite #91

gegoune opened this issue Feb 19, 2023 · 6 comments · Fixed by #148

Comments

@gegoune
Copy link

gegoune commented Feb 19, 2023

In spirit of nvim-telescope/telescope.nvim#2290, replacing dependency on sqlite with vim.json would make telescope-frecency easier to run on remote machines and require users to install fewer plugins.

I am not sure if it is at all possible for that extension, but the only difference I see is removing records for files that no longer exist.

@gbroques
Copy link

Related to #63.

To @gegoune's point, removing the sqlite dependency would make this plugin easier to setup.

@delphinus
Copy link
Member

I think string.dump to compile Lua table into Lua bytecode is better than JSON. It needs no decoding after loading DB. Nevertheless, SQLite is so fast that it probably slows down if we use other solutions. 🤔

@delphinus
Copy link
Member

I have introduced the way for storing data without sqlite.lua. You can use use_sqlite = false option for it and :FrecencyMigrateDB to migrate SQLite DB into string.dump()'s one. Try it.

For the time being, I will do dog-fooding this and consider use_sqlite = false can be the default setting.

@gegoune
Copy link
Author

gegoune commented Aug 31, 2023

Thanks! It seems to mostly work fine with couple of hiccups.

I get A inserted into prompt on first invocation. Worries fine after. There was an issue on main telescope repo regarding something like this.

I get diffview:// buffers listed as well. I am not ignoring them but didn't before either and I believe they only started showing after turning of SQLite.

Command you mentioned wasn't available after updating plugin. Didn't really check why as I was fine with starting with fresh history.

Thank you very much for your hard work! Really appreciated. What do you think about this change so far? Do you think it's beneficial or not?

Edit: I have removed database that got created based in oldfiles and cleaned it up. Stroked through items above are no longer an issue.

Edit2: it really does feel way snappier now. Excellent job!

@delphinus
Copy link
Member

Thank you for using! It seems that I consider migration process much if I make native mode (use_sqlite = false) be the default value.

I also had trial of this for weeks, noticed some problems below.

  1. It reads fresh results from DB every time with the current logic (with SQLite). But with native mode, it reads results at the first launch only. It does not re-read DB if it is updated newly opened files. Also new entries by other Neovim instances does not affect until Neovim restarts.
  2. Native mode has less reliability than SQLite mode. The read/write process is protected by exclusive lock (wx flag in uv.fs_open), but it is not flawless.

I think the former one can be solved by checking the timestamp of DB file, but the latter one cannot be solve perfectly. It is better that I create a command, such as FrecencyRemoveLock, to remove the lock file (rm file_frecency.bin.lock) if it remains by accident.

Anyway, I feels native mode works well. It is faster than SQLite mode, and it does not lock Neovim main loop because all the process are working asynchronously. When such problems are solved, I make native mode be the default option.

@delphinus
Copy link
Member

I have introduced a feature to reload DB asynchronously when the DB file is updated. This means :Telescope frecency always returns the latest results even if another Neovim process have updated the DB file.

This is implemented by uv.new_fs_event to watch DB file's changes. So it works even on Windows. I also added tests on macOS and Windows to check these features in #143.

Next, I will implement more robust migration feature and write a guide document for it. Then at last, we can close this issue.

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 a pull request may close this issue.

3 participants