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

Refactor. Maybe something from that will be helpful #42

Closed
wants to merge 43 commits into from

Conversation

AgatZan
Copy link
Contributor

@AgatZan AgatZan commented Sep 2, 2024

  • refactor(utils): remove useless Slash(), make const instead ( also maybe just global os_sep )

  • refactor(utils): remove initDir.
    Because users need to read the README and think about the path, if they ignore it, their problem. It's better than notify every <cmd>Sratch<cr> that you are using an existing directory. Or like case cash to somewhere and check the hash. ¯\(ツ)

  • fix(utils): get width/height using vim.api.win instead deprecated option or hacky option_value

  • fix(windows.utils): requireDir when create uuidgen doesnt exist in windows by default, so instead just os.time. Timestamp unique ¯\(ツ)

  • feat(telescope): add local keys

  • feat(config): MANUAL_INPUT now user customizable

  • perf(utils): instead using fn use vim.uv. It's necessary to profile, but something tells me that recursion is not even fast.

  • chore: consistent everywhere return M instead at one place return {...funcs} at another place return M.

  • chore(type): fix Scratch.WindowCMD at config,

  • feat(api): Refactor all API. 1 call function inline, because why do they exist?(except for rather bulky ones, like utils.scandir) Move something from one file to another.

    • feat(config): instead of creating bicycles using win_conf from nvim_win_open

    • For example all open_*_scratch now findBy* and placed in default_finder.

    • Now users can specify they own input/selector examples in default_providers.lua. I think it's "shoot a cannon at sparrows", but better than hardcoding.

style = "minimal",
border = "single",
title = "",
title = title,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it was forgotten

function M:scratchByName(filename, win_conf, content, local_keys, cursor)
local abs_path = self.base_dir .. filename
local fto = {}
for i in filename:gmatch("([^%.]+)") do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string.split by separator (in this place it ".")

---@return string[]
function M:get_all_filetypes()
local combined_filetypes = {}
local cash = {}
Copy link
Contributor Author

@AgatZan AgatZan Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of sneaking through all combined_filetypes each time we append an item, just caching with ~O(1)

coroutine.wrap(function()
local ft = selector_filetype(filetypes)
self:scratchByType(ft, win_conf, content, local_keys, cursor)
end)()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inspired by https://github.com/mfussenegger/nvim-dap/blob/66d33b7585b42b7eac20559f1551524287ded353/lua/dap/ui.lua#L55

For situations like vim.ui or something else, I don't know a better way.

end

-- ---@param opts Scratch.LocalKey[]
-- function M:scratchOpen(opts)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's useless to set up some built-ins this way. If a user wants to use one of them, he must call one of them. And specify these way it's to sign to update this every time u add. In a good way, it would be nice to just give a draft of what to look like without passing unnecessary openers.

-- table.sort(files, function(a, b)
-- return vim.fn.getftime(scratch_file_dir .. vim.g.os_sep .. a)
-- > vim.fn.getftime(scratch_file_dir .. vim.g.os_sep .. b)
-- end)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user can want to specify their own comparator or even not sort.

end

---@param base_dir string path to scandir
function M.findByFzf(base_dir)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use it. So I don't know how it works.

@@ -0,0 +1,94 @@
local M = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this is unnecessary, so it can be commented and put in the doc.

@@ -0,0 +1,49 @@
local utils = require("scratch.utils")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same like default_finder

@@ -0,0 +1,48 @@
local M = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actor.lua can be placed here without any problems.

@@ -0,0 +1,39 @@
local M = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't change this it's. My fault and git skill issue

end
local config_data = config.getConfig()
local scratch_file_dir = config_data.base_dir
local p = Path:new({ scratch_file_dir, file_name, sep = vim.g.os_sep })
Copy link
Contributor Author

@AgatZan AgatZan Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think `vim.g.os_sep' is the best way of doing that. I don't know why it is not std const. It can be found in so many ways, but why do even all people need to create a bicycle? Thinking out loud.

@@ -30,7 +30,7 @@ end
---@param win_conf? vim.api.keyset.win_config
---@param content? string[]
function M:scratchByType(ft, win_conf, content, local_keys, cursor)
local abs_path = self.base_dir .. utils.gen_filename(ft)
local abs_path = self.base_dir .. utils.get_abs_path(ft)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can be replaced by user customizable version

vim.api.nvim_buf_set_name(buf, abs_path)
--- BUG: if buffer exist throw error, so need find existed buffer with that name
--- TODO: REFACTOR THAT
local suc = pcall(vim.api.nvim_buf_set_name, buf, abs_path)
Copy link
Contributor Author

@AgatZan AgatZan Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know if it needs to be fixed or not. Because this function is called when creating a new file.

vim.api.nvim_create_user_command("ScratchOpen", scratch_api.openScratch, {})
vim.api.nvim_create_user_command("ScratchOpenFzf", scratch_api.fzfScratch, {})
vim.api.nvim_create_user_command("ScratchWithName", scratch_api.scratchWithName, {})
vim.api.nvim_create_user_command("ScratchOpen", function()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScratchOpen is supposed to use user's configured picker to find and open scratchFiles(file_picker = "fzflua",, they can choose from telescopefzf-lua and the default vim.ui).
But after refactor, this configuration is not working as how it works before.

Copy link
Contributor Author

@AgatZan AgatZan Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the point here is that in this case, if scratchOpen is used, it will be exclusively with pre-installed ones. It will be easy to make file_picker type like "fzf"|"telescope"|"native"|fun(scratch_file_dir:string)
And then make

function M:scratchOpen()
  local typ = type(self.file_picker)
  if typ == "string"
    require("scratch.default_finder." .. self.file_picker)(self.scratch_file_dir)
  elseif typ == "function"
    self.file_picker(self.scratch_file_dir)
  else
    vim.notify("Incorrect type")
  end
end

And if they don't suit? I'll have to write it myself and they will do as I describe below. The same functionality can be implemented. By making file_picker a function, but in this case the question is whether it is necessary in the config.

For me, you can just put them in a separate file, ala default_finders.lua or something else. You call with only 1 parameter scratch_file_dir you anyway get access to config, so i don't really know why not just. require('default_finders').native(vim.g.scratch_config.scratch_file_dir)

vim.api.nvim_create_user_command("ScratchOpen", function()
require("scratch.default_finder").findByNative(vim.g.scratch_actor.scratch_file_dir)
end, {})
vim.api.nvim_create_user_command("ScratchOpenFzf", function()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScratchOpenFzf should Grep the contents of scratch files and open a selected file.

Maybe I should rename it to ScratchOpenGrep

}
local Actor = require("scratch.actor")
---@type Scratch.Actor
vim.g.scratch_actor = vim.g.scratch_actor or setmetatable(default_config, Actor)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very fond of set a metatable to a config, which to me, looks like turn a config into a object instance.

I would accept to init a actor with a config pass to it's constructor method.

require("scratch.default_finder").findByTelescope(vim.g.scratch_actor.scratch_file_dir)
end, {})
vim.api.nvim_create_user_command("ScratchWithName", function()
vim.g.scratch_actor:scratchWithName()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to call vim.g.scratch_actor:method_name() but seems all of those methods are nil.

Is it works on your side? or am I missing some extra config?

Copy link
Contributor Author

@AgatZan AgatZan Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why but when u set metatable it don't insert method into 'vim.g`, so 2 way to fix.

  1. Manually add all methods. vim.g.scratch_actor.method1 = Actor.method1...(or just vim.g.scratch_actor.methods=Actor). But I don't know how it will work.
  2. Use require('scratch').setup() don't assign vim.g .

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
  filetype_details = {},
  filetypes = { "lua", "js", "py", "sh" },
  localKeys = {},
  manual_text = "MANUAL_INPUT",
  scratch_file_dir = "/Users/lintao/.cache/nvim-lintao\\scratch.nvim\\",
  win_config = {
    border = "single",
    col = 5,
    height = 26,
    relative = "editor",
    row = 2,
    style = "minimal",
    title = "",
    width = 107
  }
}
Error executing Lua callback: ...ts/oatnil/release/scratch.nvim/plugin/scratch_plugin.lua:23: attempt to call method
'scratchWithFt' (a nil value)
stack traceback:
        ...ts/oatnil/release/scratch.nvim/plugin/scratch_plugin.lua:23: in function <...ts/oatnil/release/scratch.nvi
m/plugin/scratch_plugin.lua:19>

I'm still getting this error msg

Copy link
Contributor Author

@AgatZan AgatZan Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I have error when occurred .setup() because vim.tbl_deep_extend needed table, but or user_config or vim.g.scratch_config is nil. When i make :lua vim.print(vim.g.scratch_config) it exist. Something with plugin/, maybe?

Theoretically 0 mistakes, but...

Copy link
Contributor Author

@AgatZan AgatZan Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably i don't know lua but, wat?

local foo = {}
function foo.a()
  print("bbbb")
end
foo.key = "value"
local mt = { __index = foo }
local bar = setmetatable({ __index = foo }, foo)
bar.key2 = "value2"
print(bar.key) -- nil
bar.a() -- errr
local foo = {}
function foo.a()
  print("bbbb")
end
foo.key = "value"
local mt = { __index = foo }
local bar = setmetatable({}, mt) --- that changed
bar.key2 = "value2"
print(bar.key) -- value
print(mt.key) -- but nil
bar.a() -- bbbb

Maybe there is a reason for it. So the best way to fix it is. Actor:new()

By the way, i remade api.lua so all work can be done using it with vim.g.scratch_config

@LintaoAmons
Copy link
Owner

Hi @AgatZan

I appreciate the effort put into this task, but I would like to outline some requirements for the changes to be accepted:

  1. Seamless User Experience: Since this is a significant refactor, it is essential that the changes do not disrupt the existing user experience. The final implementation should be fully compatible with what users are accustomed to.

  2. Incremental Improvement: We don’t need to achieve the ideal final state in a single implementation. If any aspects of the refactor aren’t perfect at this stage, we can address them in future pull requests (PRs).

  3. Plugin File Structure: It is crucial to maintain the existing plugin file structure. Specifically, we must keep plugin/scratch_plugin.lua, as I consider it the entry point of the plugin. Additionally, lua/scratch/init.lua should remain as the namespace, and lua/scratch/config/ will house configuration-related files.

@AgatZan
Copy link
Contributor Author

AgatZan commented Sep 8, 2024

  1. Seamless User Experience
  2. Incremental Improvement
  3. Plugin File Structure
  1. As I wrote earlier, I do not understand exactly the above solution with a choice of pre-installed file_finders and window_cmd, so the main goal of PR was to fix it and refactor function structure that i found strange. I think I'm done with it.
  2. I don't really understand it. I make changes and fix bugs that these changes make.
  3. I look at nvim man and also don't understand what plugin stands for and when I append something to that with the lazy.nvim plug-manager in dev mode, it always throws an error.. Only one thing that is written about it is that it is for vim plugins. In the same piggy bank and the need to duplicate function names in init.lua maybe the same way is just move all API to api.lua. And I think the necessity of it is dictated by a non-consistent functional structure.

Anyway, I respect your decision and that's why I included "maybe something will be helpful" in the title of the PR

@AgatZan AgatZan closed this Nov 23, 2024
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.

2 participants