tools: adopt llformat formatting targets#1136
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces llformat as the new tool for managing vertical whitespace in handwritten Go code, updating the project's configuration, Makefile, and CI tools accordingly. The review identified a performance improvement for the fmt target in the Makefile, a typo in the daemon startup error message, and a robustness issue in the file listing script regarding filenames with special characters.
| fmt: $(LLFORMAT_BIN) | ||
| @$(call print, "Formatting all handwritten Go source.") | ||
| @./scripts/llformat-files.sh all | \ | ||
| xargs -0 -n 1 $(LLFORMAT_BIN) -w |
There was a problem hiding this comment.
Invoking the formatter with xargs -n 1 is inefficient as it forks a new process for every single file. Formatter tools like llformat typically support multiple file arguments, so you should allow xargs to pass as many files as possible in a single invocation to improve performance.
xargs -0 $(LLFORMAT_BIN) -w
| "(standalone or embedded in lightning-terminal) is"+ | ||
| "running", err) | ||
| "(standalone or embedded in lightning-terminal) "+ | ||
| "isrunning", err) |
| git diff --name-only --diff-filter=ACMR "$base"...HEAD | ||
| git diff --name-only --diff-filter=ACMR | ||
| git diff --cached --name-only --diff-filter=ACMR | ||
| git ls-files --others --exclude-standard | ||
| } | sort -u | while IFS= read -r file; do |
There was a problem hiding this comment.
The list_changed function is not as robust as list_all because it relies on newline-separated output from git and sort. While rare in Go projects, filenames containing spaces or special characters could cause issues. It is safer to use null-terminated strings throughout the pipeline, especially since the output is eventually consumed by xargs -0.
| git diff --name-only --diff-filter=ACMR "$base"...HEAD | |
| git diff --name-only --diff-filter=ACMR | |
| git diff --cached --name-only --diff-filter=ACMR | |
| git ls-files --others --exclude-standard | |
| } | sort -u | while IFS= read -r file; do | |
| { | |
| git diff --name-only -z --diff-filter=ACMR "$base"...HEAD | |
| git diff --name-only -z --diff-filter=ACMR | |
| git diff --cached --name-only -z --diff-filter=ACMR | |
| git ls-files -z --others --exclude-standard | |
| } | sort -z -u | while IFS= read -r -d '' file; do |
|
@claude please review the changes to the code that are introduced in this PR. Can you confirm this the changes are style and formatting changes only? Or are there any logic or behavioral changes introduced? |
Code Review: tools: adopt llformat formatting targetsOverviewThis PR adopts Tooling InfrastructurePositive:
Concerns:
Linter ConfigurationPositive:
Concern:
Bug Fix: Duplicate "sweep" in
|
| Category | Assessment |
|---|---|
| Correctness | ✅ The duplicate-word bug fix is correct and well-scoped |
| Tooling | ✅ Solid — minor dead-code and portability nits |
| Linter config | ✅ Change is well-justified; gofmt compatibility dependency worth noting |
| Formatting baseline | ✅ Machine-generated; no logic changes |
| Test coverage | N/A — pure tooling/formatting PR |
| Security | N/A |
Suggested action items before merging:
- Remove or comment the now-unused
GOIMPORTS_BIN/GOIMPORTS_PKG/$(GOIMPORTS_BIN)targets from the Makefile (or document why they're kept). - Confirm
llformatapplies thegofmt -ssimplifications, or document the delta. - Fix the pre-existing missing space (
, fees:) inliquidity/parameters.gowhile the string is already being touched. - Consider adding
--togit diffinvocations inscripts/llformat-files.sh.
|
@hieblmi, remember to re-request review from reviewers when ready |
| github.com/ashanbrown/forbidigo/v2 v2.3.0 // indirect | ||
| github.com/ashanbrown/makezero/v2 v2.1.0 // indirect | ||
| github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect | ||
| github.com/bhandras/llformat v0.0.0-20260505085734-44b3ea10b9bb // indirect |
There was a problem hiding this comment.
I've added a bunch of fixes, could you pls update to v0.1.1-beta and regenerate the baseline?
28fa32e to
9d589b4
Compare
|
Addressed #1136 (comment). Updated/regenerated the branch with github.com/bhandras/llformat v0.1.1-beta, rebased it onto current upstream/master. |
9d589b4 to
c0f1972
Compare
| // assets daemon. | ||
| func DefaultTapdConfig() *TapdConfig { | ||
| defaultConf := tapcfg.DefaultConfig() | ||
|
|
There was a problem hiding this comment.
Can you pls regenerate this commit with only the baseline and then add the lint diff as a separate commit? This'd help with just mechanical validation that the diff is indeed created with llformat.
There was a problem hiding this comment.
This now contains the lint-diff only.
c0f1972 to
c2998aa
Compare
Summary
Adopt
llformatas the repository formatting path while keeping the formatter invocation equivalent to vanillallformatdefaults. The Makefile only passes-wso files are written in place; all formatting behavior remains owned byllformatitself.This PR is split into four commits:
tools: add llformat formatting targetsgithub.com/bhandras/llformat/cmd/llformatin the tools modulemake fmtfor all handwritten Go filesmake fmt-changedfor branch, staged, unstaged, and untracked Go changesscripts/llformat-files.shtools: tune lint for llformat adoptionformat: apply llformat baselinemake fmtresult across handwritten Go filesliquidity: fix duplicate sweep labeldupwordafter llformat joined adjacent string fragmentsValidation
make fmtmake fmt-changed base=upstream/mastermake mod-checkmake build tags=devmake lintGOCACHE=/private/tmp/loop-go-build-cache make unitNotes
The file-selection helper excludes generated Go outputs, vendor, third_party, and testdata paths before invoking
llformat. It does not pass any non-default formatter options.