-
-
Notifications
You must be signed in to change notification settings - Fork 603
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(git): common code to a generic git cll #2551
Conversation
66486c5
to
10c60d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for the very limited testing I've done.
I appreciate all you efforts to improve the codebase, however this has gone much further than a refactor.
Please restrict this PR to a simple refactor: move existing code and change only what is absolutely needed.
lua/nvim-tree/git/cli.lua
Outdated
|
||
M.git = M.cli | ||
-- This is a trick to workaround lua_lsp_server limitation on metatable | ||
-- function type declarations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How and why does this exceed limitations?
We should resolve the root cause rather than working around limitations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lua-language-server doesn't have @function
+ @params
to force a type of a variable. The above sets the same LSP completion signature for both M.git
and M.cli
.
You can contribute here: https://github.com/LuaLS/lua-language-server
Tracking issue: LuaLS/lua-language-server#1456
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to force type to coerce the signatures?
Can you not just use unique names i.e. do not overload .git
?
Unnecessary fragile code to work around language server limitations is not acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
M.git = setmetatable({}, {
__call = function(_, ...)
return M.cli(...)
end,
})
The above creates M.git
both as namespace and callble M.git()
property and annotation make sure that LSS recognized this:
M.git()
M.git.mv()
M.git.is_tracked()
This is actually used in my actions.git
PR to extend CLI. with few commands.
Refactored in next revision.
A side note on dependency injection & code portability & reusability--- ./bar.lua
local Foo = require("foo")
M.Foo = Foo
...
function M.public_fn ()
M.Foo.xyz()
end
return M The above is a demo of variable reexport. It's a programming approach to make code extensible and therefore - reusable (or hackable). It allows reassignation of basic functions / structures (injection) by downstream consumer of In a typical use case consumer of --- ./baz.lua
local b = require("bar")
b.Foo = FooExtended
b.public_fn(...)
This is useful in cases where you want to extend |
10c60d7
to
efa2265
Compare
I do not appreciate being lectured to about design patterns. I am rapidly losing patience. All codebases have flawed structures, antipatterns, inefficiencies, repetition, safety issues etc. etc. Project developers are very aware of these issues however lack the time and priority to resolve them. No codebase will ever be perfect. Please prove your skills first via several contributions. We may then speak, respectfully, about improvements. |
Please push discrete commits to the PR rather than amending or squashing etc. A changelog is necessary so that the reviewer can see what has changed since the last review. |
Removing extra input arguments checks is really dumb. If we don't check input, we risk internal consumers of the arguments to become error-ridden. This is not how mature programming is done lol. I will remove them in next revision for now, but it's really dumb and heavy nitpcking. This will ensure that the code is less error-prune. |
I strongly suggest you that don't send comments like this any more. Let this one be the last one, ok? |
Runner: separate generic `git.cli` git command into a wrapper Runner: reuse separate wrapper Git cli: add luadoc annotations, basic args check, logs
efa2265
to
40521e0
Compare
@hinell you are no longer welcome in the nvim-tree community. Your behaviour is rude, uncooperative, condescending, argumentative and very disrespectful. This behaviour is pathological and I don't foresee your changing. You have wasted a lot of the nvim-tree community's time with your unwanted, misinformed intrusions and naive, unsolicited advice. Listen mate: you need to take a good hard look at yourself and Grow Up. Any potential employer seeing your github behaviour will run a mile. |
For the record: Internal functions should handle all inputs gracefully. Throwing exceptions on argument mismatch is unacceptable unless they are explicitly handled by the caller. Arguments are "checked" when they are nilable or have varying type. Some pieces of data are assumed and known to be valid and complete, such as options. These are not checked at all as it is impractical and of no real value. |
git-integration-refactor.mov
git.cli
thin-client for git command