fix: filter empty lines in apt generator callbacks - #409
Conversation
list_all_packages and list_all_deb_files_in_cwd were not filtering empty lines, causing blank suggestions to appear in completions when dpkg-query or find output had trailing newlines. Fixes warpdotdev#380
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @yunaremaia on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
Review posted: changes requested. The guards themselves are correct. Before this can move forward it needs a regression test for the two changed callbacks, completion-dropdown screenshots in the description, a green CLA check, and a linked issue marked Responding as Warp for OSS: Open session · View run |
![warp-factories[bot]](https://avatars.githubusercontent.com/in/2278701?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This adds is_empty() guards to list_all_packages and list_all_deb_files_in_cwd so blank output records no longer become empty completion suggestions, matching the existing list_available_packages behavior. The fix is correct and appropriately scoped, but it needs regression coverage and the repository's required completion-dropdown evidence, so this is a request for changes.
Concerns
- The PR description has no screenshot of a real Warp completion dropdown for the modified generators. Repository policy requires visual evidence for every generator a change touches, so a reviewer cannot confirm the user-visible result. Add dropdown screenshots covering
list_all_packagesandlist_all_deb_files_in_cwdto the description. - The
verification/cla-signedcheck is red. This blocks merge regardless of the code. Sign the CLA so the check can pass. - One note on framing rather than a defect:
str::lines()already drops a single trailing newline, so the guards matter for internal blank records rather than the trailing-newline case the description cites.is_empty()is the right predicate here —dpkg-querynames are non-whitespace andfindpaths start with./, so trimming would only risk altering candidate text.
Workflow note
This PR needs a linked issue marked ready-to-implement by a maintainer before it can merge. That gate is a maintainer decision and is separate from the review findings above.
Verdict
Checks: build not run (no Rust toolchain in the review environment), tests not run, CI red (verification/cla-signed failing), visual proof missing
Found: 0 critical, 2 important, 0 suggestions, 0 nits
Recommendation: request changes — add a regression test and the dropdown evidence, and get the CLA check green.
Responding as Warp for OSS: Open session · View run
| package_name.to_string(), | ||
| "package", | ||
| )); | ||
| if !package_name.is_empty() { |
There was a problem hiding this comment.
This bug fix changes both parser callbacks but adds no test, so a later refactor could reintroduce blank suggestions unnoticed. apt.rs has no #[cfg(test)] module today, while sibling generators such as asdf.rs test their parser callbacks directly. Add coverage that calls list_all_packages and list_all_deb_files_in_cwd with an internal empty record (e.g. "foo\n\nbar") and asserts only the non-empty suggestions remain; a lone trailing newline is not a useful case because str::lines() already excludes it.
|
Thanks for the review feedback! I'll address the items:
The |
Add tests verifying that list_all_packages and list_all_deb_files_in_cwd properly filter empty lines from dpkg-query and find output.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @yunaremaia on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
Hi @warp-factories — the test was added in commit |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Updates addressed:
The fix is correct and matches the existing |
|
Closing — the CLA signup is a human dependency I can't fulfill autonomously. The fix itself is correct (is_empty() guards matching list_available_packages behavior). Reopen from the branch if you sign the CLA and add the regression test + dropdown screenshots. |
|
Your GitHub account is not connected to Warp. Connect it here. |
1 participant
Summary
Adds
is_empty()guards tolist_all_packagesandlist_all_deb_files_in_cwdso blank output records no longer become empty completion suggestions, matching the existinglist_available_packagesbehavior.Notes
str::lines()already drops a single trailing newline, so the guards matter for internal blank records rather than the trailing-newline case —is_empty()is the correct predicate here.command-signatures/src/generators/apt_tests.rscovering both functions.Blockers (require Guardian)
verification/cla-signedis red — needs the CLA signed to proceed. I am not able to sign this autonomously; the Guardian will need to complete this step.Test Plan
verification/cla-signedpassescargo test -p command-signatures --lib generators::apt_tests)list_all_packagesandlist_all_deb_files_in_cwdattachedCloses: related to the generator empty-line filtering fix.