Intro
I’m a developer passionate about the open-source community.
For a long time I spent most of my time working or hanging out. But recently, I sent several small PRs to neovim and golang/tools — and that felt great.
So I refreshed my blog using mdbook with the catppuccin theme, to document what I’ve worked on and what I’m building.
To Find Me
You can find me by searching acehinnnqru on social media:
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 the Neovim project.
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 introduced me to a Lua pattern I hadn’t used 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. -
Large codebases are approachable: Contributing to projects like Neovim is less intimidating than it seems. Start with well-defined issues and treat reviewer feedback as a learning opportunity.
PR: Set Correct ’] Mark When Pasting Chunks
Background
I discovered this issue while browsing through the Neovim issue tracker.
Implementation Journey
The final fix was only a few lines of code, but the review process spanned 20+ commits.
Review Iterations
The review process was more challenging than the bug itself. After my initial implementation, @zeertzjq suggested improvements to the conditional logic. I experimented with various if branch arrangements, each time believing the code was close to ready.
When @zeertzjq brought @justinmk into the review, I was confident we were nearly done. But @justinmk proposed a different approach—one that, ironically, resembled my very first attempt. This sparked a cycle of toggling between versions, each making sense on its own but none winning consensus.
I eventually realized my mistake: I failed to document the rationale behind my implementation choices. Without explaining why I chose a particular approach, each reviewer naturally had their own preferences, creating unnecessary back-and-forth.
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 approaches are possible, explain why you chose yours. Reviewers can’t read your mind — a short rationale prevents misunderstandings and wasted iteration.
-
A small diff doesn’t mean a small effort: This PR’s final change was a few lines, but it took 20+ commits to get there. Don’t underestimate the social dimension of contributing to large projects.
Golang
To the Go project and its sub-projects.
Tools
To the Go tools project.
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. Alan Donovan reviewed the code and recommended extracting rather than duplicating the function—a cleaner approach that follows the DRY principle.
Edge Case Discovery
My initial implementation collected all comments and placed them at the start node position regardless of whether mrhs was nil. 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 points to the node before the range statement, placing comments 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 about to copy a function, extract it to a shared location instead. A small refactor up front keeps the codebase maintainable and avoids divergent copies.
-
Test edge cases thoroughly: My initial implementation worked for the simple case (
mrhs == nil) but failed whenmrhs != nil. Always exercise every code path before calling it done. -
AST positioning matters: When working with Go’s AST, a single wrong node position can place your changes in entirely the wrong location. The difference between
rng.Pos()andcurPrev.Node().Pos()is subtle but critical. -
Document transformations: The inline before/after examples in the code comments help future maintainers understand what the refactoring does at a glance.