-
Notifications
You must be signed in to change notification settings - Fork 19
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(git): improve readability and maintainability #78
base: main
Are you sure you want to change the base?
Conversation
This PR contains too much content, which makes it difficult to diff. Is it possible to split it into several smaller PRs? |
Sorry, but I'm afraid this would be quite difficult, as it involves an almost complete refactor with various parts intertwined. However, I can provide some technical details:
|
If you don't mind, I will force push to the PR after I finish. The new commits include the following changes:
|
6901745
to
0a00484
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 commit's changes don't make sense.
It moves work that was originally handled in async tasks into a sync context, and for each linemode miss, it repeatedly loops to check its parent. Since linemode is invoked for all files on the page, this will lead to a significant amount of memory allocation.
The bottom line here is that any computational work should be avoided as much as possible during UI rendering.
Sorry, I didn't realize that. I will try to revert it. (Perhaps there's a better way to handle this? The introduction of 30 has increased the complexity and difficulty of understanding the code.) |
IMO this PR should just focus on adding the new feature and keep the
changes as small as possible so it’s easier to review. Improving code
readability is great, but it might be worth doing that in a separate PR
where we can dig into it a bit more and discuss it further.
…On Mon, Mar 3, 2025 at 3:04 PM PFiS ***@***.***> wrote:
Sorry, I didn't realize that. I will try to revert it. (Perhaps there's a
better way to handle this? The introduction of 30 has increased the
complexity and difficulty of understanding the code.)
—
Reply to this email directly, view it on GitHub
<#78 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEFWFIHLZSUJZ5BOL6EAUCL2SP5GZAVCNFSM6AAAAABYE6ER3WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJTGQ2TONBVGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
[image: PFiS1737]*PFiS1737* left a comment (yazi-rs/plugins#78)
<#78 (comment)>
Sorry, I didn't realize that. I will try to revert it. (Perhaps there's a
better way to handle this? The introduction of 30 has increased the
complexity and difficulty of understanding the code.)
—
Reply to this email directly, view it on GitHub
<#78 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEFWFIHLZSUJZ5BOL6EAUCL2SP5GZAVCNFSM6AAAAABYE6ER3WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJTGQ2TONBVGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Sorry for wasting your time, and please forgive my unfamiliarity. I will revert the commits introducing the new feature, making this PR solely focused on discussing improvements to code readability and maintainability. |
0a00484
to
d5b2fed
Compare
The rollback is complete, and I have translated the original logic according to the latest type definitions. I will continue to explore how to remove the |
Well, I failed — I couldn't come up with any method other than using an additional flag. However, I made some optimizations:
|
} | ||
|
||
---@type Config | ||
local Config = { |
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.
What does capitalizing the first letter mean? Is it a constant or a variable? It seems that the entire repository does not follow such a naming convention
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.
Oh, that's a habit of mine — I usually treat config as an independent module (when writing Neovim plugins), and capitalizing the first letter helps emphasize its independence. Perhaps we should change this naming to make it consistent with the others?
} | ||
|
||
---@type table<string, Status> | ||
local PATTERNS = { |
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.
Any reason to change their order?
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 decision was made based on importance: untracked
should have a higher priority to serve as a prompt, and added
should be more important than modified
. Although ignored
is handled separately, I assigned it the highest priority to align with the context's sorting method. I also referred to the priority used in NeoTree (a Neovim plugin).
else | ||
changed[path] = sign | ||
local status, path = match(line) | ||
if status and path then |
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 is this if
statement required?
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.
According to the logic, match
will return nil
, and the LSP also suggests performing a nil check.
1ed7bd3
to
c84f893
Compare
I have only reviewed part of it, and it is still very difficult to review as the different changes are intertwined and this PR refactored almost the entire plugin, and I am not sure how to correctly review them. I'll try to split these changes into several smaller PRs later and request you review them. I will add you as a collaborator to make sure you get the credits. |
Sincerely appreciate your efforts and thank you for your patience these days. I will do my best to fulfill my part. |
|
||
-- Chosen by ChatGPT fairly, PRs are welcome to adjust them | ||
local t = THEME.git or {} | ||
local styles = { |
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.
styles
and signs
are local variables rather than being extracted to a global Config
was intentional because they are only used in setup (sync plugin).
This avoids initializing a bunch of unnecessary ui.Style
s when scheduled as an async fetcher, where they won't be used at all.
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.
I will make the changes as requested.
Additionally, why don't we prefer setting styles
in the default configuration and README
as table
instead of ui.Style
? This would simplify the process for users to move configurations into theme.toml
(just copy-pasting instead of translating) and would also avoid unnecessary ui.Style
s.
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.
If you want to represent ui.Style():bold()
as a Lua table, it would look like { modifier = 1 }
, where 1
is the binary value of the bold
style, which is not user-friendly.
And when a TOML table is converted to a Lua table, it's also converted to { modifier = X }
instead of { bold = true, underline = true, ... }
for better performance.
Hi, I'm planning to submit separate PRs over the next few days to complete the updates:
Today's will be the first. You can review them and give feedback whenever you have time, and I'll submit the next one once the previous one is merged. |
Description
nf-oct-diff
series icons from Nerdfonts and terminal colors (instead ofRGB
colors).