Intro
I am a developer, full of passion to open-source community.
In a long time I spent most of my time working or hanging around. But recently, I sent several small code PRs to neovim and golang/tools that made me felt so great.
So I renewed my blog using mdbook with catpuccin theme, to write down what I have done or working on in my life.
To Find Me
You can easily find me by searching acehinnnqru in social medias, including,
Open Source
This directory would record my what I have done or learnt in open-source community.
I would orgnize it by projects.
Neovim
To project Neovim
PR: Fix opts.title Behaviors of _make_floating_popup_size
Background
I discovered this issue while browsing through the Neovim issue tracker.
Understanding the Problem
The issue reported unexpected behavior with opts.title in _make_floating_popup_size. My investigation revealed that while opts.title worked correctly with simple string values, it failed to handle the [string, string][] format (an array of text-highlight pairs).
After discussing with the issue author, I learned that vim.lsp.util.open_floating_preview internally uses vim.api.nvim_open_win, which supports both title formats according to the Neovim API specification. The bug was clear: _make_floating_popup_size needed to calculate width correctly for both title formats.
Implementation Journey
Initial Attempt
My first implementation was functional but had critical flaws:
if opts.title then
local title = opts.title
local title_length = 0
if type(title) == 'string' then
title_length = vim.fn.strdisplaywidth(title)
else
if title ~= nil then
for i = 1, #title do
title_length = title_length + string.len(title[i][1])
end
end
end
width = math.max(width, title_length)
Issues with this approach:
- Incorrect CJK handling: Using
string.len()instead ofvim.fn.strdisplaywidth()resulted in wrong width calculations for CJK (Chinese, Japanese, Korean) characters. - Code complexity: The nested conditionals made the logic harder to follow and maintain.
Refined Solution
@luukvbaal suggested a much more elegant approach:
local title_length = 0
local chunks = type(opts.title) == 'string' and { { opts.title } } or opts.title or {}
for _, chunk in
ipairs(chunks --[=[@as [string, string][]]=])
do
title_length = title_length + vim.fn.strdisplaywidth(chunk[1])
Why this is better:
- Unified handling: Normalizes both title formats (
stringand[string, string][]) into a consistent array structure - Cleaner logic: Eliminates nested conditionals with a concise ternary expression
- Type safety: Uses Lua's type annotation syntax
--[=[@as [string, string][]]=]to help with LSP type checking
This elegant solution taught me a new Lua pattern that I hadn't encountered before.
Testing
Before submitting for review, I added a unit test following the existing test patterns:
it('considers [string,string][] title when computing width', function()
eq(
{ 17, 2 },
exec_lua(function()
return {
vim.lsp.util._make_floating_popup_size(
{ 'foo', 'bar' },
{ title = { { 'A very ', 'Normal' }, { 'long title', 'Normal' } } }
),
}
end)
)
end)
The test validates that the function correctly computes the width when given a title with multiple styled chunks.
Outcome
After pushing the changes and requesting review, @zeertzjq merged the PR successfully.
Key Takeaways
-
Communication is crucial: Discussing with the issue author helped me understand the underlying API design and expected behavior.
-
Learn from reviewers:
@luukvbaal's suggestion introduced me to more idiomatic Lua patterns and the type annotation syntax for better code quality. -
Don't be intimidated: Contributing to large codebases like Neovim is more approachable than it seems. Starting with well-defined issues and learning from reviewer feedback makes the process manageable and educational.
PR: Set Correct '] Mark When Pasting Chunks
Background
I discovered this issue while browsing through the Neovim issue tracker.
Solution Process
Although the final fix only required a few lines of code, the implementation went through many iterations, resulting in 20+ commits during the review process.
Review Iterations
The review process turned out to be more challenging than the bug itself. After submitting my initial implementation, @zeertzjq suggested improvements focusing on the conditional logic structure. I experimented with various if branch arrangements, believing I was moving in the right direction.
When @zeertzjq brought @justinmk into the review, I was confident the code was ready to merge. However, @justinmk proposed a significantly different approach—one that, ironically, resembled my very first implementation. This sparked a cycle where I found myself toggling between different versions, each making sense from a certain perspective but lacking consensus from the reviewers.
The Key Lesson
I eventually realized my mistake: I failed to document the rationale behind my implementation choices. Without a clear explanation of why I implemented it a certain way, different reviewers naturally had different preferences and suggestions. This caused unnecessary back-and-forth discussions.
Final Implementation
Here is the merged version:
--- a/runtime/lua/vim/_editor.lua
+++ b/runtime/lua/vim/_editor.lua
@@ -213,7 +213,7 @@ end
vim.inspect = vim.inspect
do
- local tdots, tick, got_line1, undo_started, trailing_nl = 0, 0, false, false, false
+ local startpos, tdots, tick, got_line1, undo_started, trailing_nl = nil, 0, 0, false, false, false
--- Paste handler, invoked by |nvim_paste()|.
---
@@ -328,7 +328,13 @@ do
-- message when there are zero dots.
vim.api.nvim_command(('echo "%s"'):format(dots))
end
+ if startpos == nil then
+ startpos = vim.fn.getpos("'[")
+ else
+ vim.fn.setpos("'[", startpos)
+ end
if is_last_chunk then
+ startpos = nil
vim.api.nvim_command('redraw' .. (tick > 1 and '|echo ""' or ''))
end
return true -- Paste will not continue if not returning `true`.
Key Takeaways
-
Document your reasoning: When multiple implementation approaches are possible, always explain why you chose your specific approach. This helps reviewers understand your thought process and reduces unnecessary iteration.
-
Be proactive in communication: If you believe your implementation is correct, provide a clear and concise summary explaining your rationale. This prevents misunderstandings and makes the review process more efficient.
Golang
To project Golang or sub projects under it.
Tools
To project Golang Tools
PR: Preserve Comments in mapsloop
Background
I discovered this issue while browsing through the Go issue tracker.
Problem Analysis
The issue reported that the mapsloop analyzer was discarding comments when refactoring map operations. The root cause was straightforward: the refactoring process wasn't collecting and preserving existing comments.
Solution Approach
The fix involved collecting comments using the allComments function and inserting them before the refactored code. However, this required some code reorganization first.
Code Restructuring
The allComments function was originally defined inside the minmax analyzer. To reuse it in mapsloop, I needed to extract it to a shared location. Thanks to Alan Donovan, who reviewed the code and recommended extracting the function rather than duplicating it—a much cleaner approach that follows the DRY (Don't Repeat Yourself) principle.
Edge Case Discovery
My initial implementation collected all comments and placed them at the start node position, regardless of whether mrhs was nil. However, this caused incorrect behavior when mrhs != nil.
The issue: When mrhs != nil (i.e., when there's a preceding make assignment), the start node points to the wrong location:
m := make(map[string]string)
^ // start node position - too late!
Placing comments here would put them in the middle of the statement rather than before it.
The fix: Use curPrev.Node() instead, which correctly points to the node before the range statement, ensuring comments are placed before the entire refactored block.
Final Implementation
Here's the complete diff:
--- a/gopls/internal/analysis/modernize/maps.go
+++ b/gopls/internal/analysis/modernize/maps.go
@@ -156,16 +156,35 @@ func mapsloop(pass *analysis.Pass) {
start, end token.Pos
)
if mrhs != nil {
- // Replace RHS of preceding m=... assignment (and loop) with expression.
- start, end = mrhs.Pos(), rng.End()
- newText = fmt.Appendf(nil, "%s%s(%s)",
+ // Replace assignment and loop with expression.
+ //
+ // m = make(...)
+ // for k, v := range x { /* comments */ m[k] = v }
+ //
+ // ->
+ //
+ // /* comments */
+ // m = maps.Copy(x)
+ curPrev, _ := curRange.PrevSibling()
+ start, end = curPrev.Node().Pos(), rng.End()
+ newText = fmt.Appendf(nil, "%s%s = %s%s(%s)",
+ allComments(file, start, end),
+ analysisinternal.Format(pass.Fset, m),
prefix,
funcName,
analysisinternal.Format(pass.Fset, x))
} else {
// Replace loop with call statement.
+ //
+ // for k, v := range x { /* comments */ m[k] = v }
+ //
+ // ->
+ //
+ // /* comments */
+ // maps.Copy(m, x)
start, end = rng.Pos(), rng.End()
- newText = fmt.Appendf(nil, "%s%s(%s, %s)",
+ newText = fmt.Appendf(nil, "%s%s%s(%s, %s)",
+ allComments(file, start, end),
prefix,
funcName,
analysisinternal.Format(pass.Fset, m),
The diff shows two key improvements:
- Comment preservation: Uses
allComments(file, start, end)to collect comments from the original code - Correct positioning: When
mrhs != nil, usescurPrev.Node().Pos()as the start position to ensure comments are placed before the entire statement block - Enhanced documentation: Adds inline comments showing the before/after transformation, making the code more maintainable
Key Takeaways
-
Don't Repeat Yourself (DRY): When you find yourself duplicating code, extract it into a shared function. This was reinforced by Alan Donovan's review feedback—extracting
allCommentsrather than copying it made the codebase more maintainable. -
Test edge cases thoroughly: My initial implementation worked for the simple case but failed when
mrhs != nil. This highlighted the importance of considering all code paths and testing different scenarios. -
AST positioning matters: When working with Go's AST (Abstract Syntax Tree), understanding node positions is crucial. A small difference (using
rng.Pos()vscurPrev.Node().Pos()) can place your changes in entirely the wrong location. -
Document transformations: The inline before/after examples in the code comments help future maintainers understand what the refactoring does at a glance.