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

feat(storage) optional persistant history #20

Merged
merged 5 commits into from
Oct 3, 2021
Merged

Conversation

AckslD
Copy link
Owner

@AckslD AckslD commented Sep 3, 2021

No description provided.

lua/neoclip/db.lua Outdated Show resolved Hide resolved
@AckslD AckslD changed the title feat(storage) optional persistant history WIP: feat(storage) optional persistant history Sep 3, 2021
lua/neoclip/db.lua Outdated Show resolved Hide resolved
lua/neoclip/db.lua Outdated Show resolved Hide resolved
lua/neoclip/db.lua Outdated Show resolved Hide resolved
lua/neoclip/settings.lua Outdated Show resolved Hide resolved
@kkharji
Copy link
Contributor

kkharji commented Sep 5, 2021

I'm testing it, there are a serious bug kkharji/sqlite.lua#112 😨. I'll get back to shortly you

@AckslD
Copy link
Owner Author

AckslD commented Sep 5, 2021

I'm testing it, there are a serious bug kkharji/sqlite.lua#112 😨. I'll get back to shortly you

Oh I see, thanks for letting me know :)

@kkharji
Copy link
Contributor

kkharji commented Sep 6, 2021

Okay it seems like there wasn't any issue and that instead of passing entries I passed self.

local has_sqlite, sqlite = pcall(require, "sqlite")
if not has_sqlite then
  print "Couldn't find sqlite.lua. Cannot use persistent history"
  return nil
end

---@class NeoclipEntry @entry content
---@field regtype string
---@field contents string[]
---@field filetype string

---@class NeoClipTable: sqlite_tbl
local M = sqlite.tbl("neoclip", {
  id = true,
  regtype = "text",
  contents = "luatable",
  filetype = "text",
})

function M:init(config)
  config = config or {}
  local db_path = config.db_path or vim.fn.stdpath "data" .. "/databases/neoclip.sqlite3"
  self:set_db(sqlite.new(db_path)) --- directory ensuring should be out of this module
end

M:init() --- called outside the module to set user db path

---Insert new neclip entry
---@param entry NeoclipEntry[] | NeoclipEntry
function M:insert(entry)
  -- do some pre processing before inserting a row
  return self:__insert(entry)
end

M:insert {
  {
    contents = { "line 1", "  line 2", "line 3" },
    filetype = "lua",
    regtype = "l",
  },
  {
    contents = { "some other string" },
    filetype = "lua",
    regtype = "l",
  },
}

---Get content using {query} if non-nil
---@param query sqlite_query_select|nil @(look into lua-dev.nvim to get autocomletion)
---@return NeoclipEntry[]
function M:get(query)
  local entries = M:__get(query)
  -- do some sorting or stuff you need.
  return entries
end

print(vim.inspect(M:get()))

return M

@AckslD
Copy link
Owner Author

AckslD commented Sep 6, 2021

@tami5 Hmm, I see, so is this branch working for you now? I'm not sure I fully understood what the difference was.

@kkharji
Copy link
Contributor

kkharji commented Sep 7, 2021

@tami5 Hmm, I see, so is this branch working for you now? I'm not sure I fully understood what the difference was.

Yes this worked for me. the pervious implementation I couldn't figure out what's wrong. Does this work on you machine

@AckslD
Copy link
Owner Author

AckslD commented Sep 12, 2021

@tami5 This current branch does not work for me for some reason, but I haven't had time yet to look into it again. I should try the code-sample you shared above and see if it works better 👍

@AckslD
Copy link
Owner Author

AckslD commented Sep 14, 2021

@tami5 I tried this again and using your code above but cannot get it working. I first tried as it is and then it complained that the table object doesn't have the method __get. I then tried to instead use the default get but then it complains that the table does not exist.

@kkharji
Copy link
Contributor

kkharji commented Sep 15, 2021

@tami5 I tried this again and using your code above but cannot get it working. I first tried as it is and then it complained that the table object doesn't have the method __get. I then tried to instead use the default get but then it complains that the table does not exist.

Yah the lsp error is because __get and other private function is not expose to emmy lsp. but it works fine. Try to delete db file. I just pasted the content and did luaf % and it works okay

@AckslD
Copy link
Owner Author

AckslD commented Sep 15, 2021

Not lsp error, an actual runtime error. I did delete the db file. Not sure what's going on here.

@kkharji
Copy link
Contributor

kkharji commented Sep 15, 2021

Not lsp error, an actual runtime error. I did delete the db file. Not sure what's going on here.

this really weird, how did u install sqlite, luarocks? and is it update to latest version?

@AckslD
Copy link
Owner Author

AckslD commented Sep 25, 2021

@tami5 I came back to this today. If I try to run the above example exactly as is using luafile % I get the error:

E5113: Error while calling lua chunk: .../site/pack/packer/start/sqlite.lua/lua/sqlite/assert.lua:28: sqlite.lua: can not execute insert
, neoclip doesn't exists.

Also if I delete the file beforehand.

I installed sqlite using sudo pacman -S sqlite.

My sqlite.lua plugin is at commit d1d8df3.

@kkharji
Copy link
Contributor

kkharji commented Sep 25, 2021

@tami5 I came back to this today. If I try to run the above example exactly as is using luafile % I get the error:

E5113: Error while calling lua chunk: .../site/pack/packer/start/sqlite.lua/lua/sqlite/assert.lua:28: sqlite.lua: can not execute insert
, neoclip doesn't exists.

Also if I delete the file beforehand.

I installed sqlite using sudo pacman -S sqlite.

My sqlite.lua plugin is at commit d1d8df3.

Oh I can confirm this bug, let me investigated and fix it. Thanks

kkharji added a commit to kkharji/sqlite.lua that referenced this pull request Sep 25, 2021
This what happens with dealing with function that can take self or
other.

Refs: AckslD/nvim-neoclip.lua#20
@kkharji
Copy link
Contributor

kkharji commented Sep 25, 2021

wow schema wasn't even getting passed 😵‍💫.

Thanks @AckslD, I would have never found such a bug

@AckslD
Copy link
Owner Author

AckslD commented Sep 25, 2021

Oh I see, great that you found it :) It might have been related to another issue I had when trying something else, namely I did something like:

local db = sqlite.new(db_path)
db:open()
local tbl = sqlite.tbl("neoclip", {
    id = true,
    regtype = "text",
    contents = "luatable",
    filetype = "text",
}, db)
tbl:set_db(db)

ie I set the db twice.

but then when calling get I got:

site/pack/packer/start/sqlite.lua/lua/sqlite/helpers.lua:71: bad argument #1 to 'next' (table expected,
got function)

However if I didn't pass db to sqlite.tbl I didn't get this error.

@kkharji
Copy link
Contributor

kkharji commented Sep 25, 2021

Hmmm this shouldn't happen, users should be able change/switch db at runtime without issues, let me open an issue to track it later.

FYI: db:tbl("neoclip", {...}) to avoid explicitly injecting a db at tbl definition.

@AckslD
Copy link
Owner Author

AckslD commented Sep 26, 2021

Thanks for the fix @tami5! Things seems to work as intended now 🎉

@AckslD AckslD changed the title WIP: feat(storage) optional persistant history feat(storage) optional persistant history Sep 26, 2021
@steinbrueckri
Copy link

@AckslD @tami5 THX for your work, you are awesome!

@AckslD AckslD merged commit 56d2793 into main Oct 3, 2021
@AckslD AckslD deleted the 16-persistant-history branch October 3, 2021 13:08
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.

3 participants