Strip non printable characters from locators#1720
Conversation
Walkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/Srclib/TypesSpec.hs`:
- Around line 19-22: The renderLocator test currently validates the main use
case with multiple control characters but lacks edge case coverage. Consider
adding two additional test cases to the renderLocator describe block: one test
verifying that renderLocator correctly handles a locator with no non-printable
characters present (regression safety), and another test covering the edge case
where fields become empty or nearly empty after stripping non-printable
characters. These additions will improve the robustness of the test suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08790897-9ffe-4a52-8e8c-b585157de8fe
📒 Files selected for processing (2)
src/Srclib/Types.hstest/Srclib/TypesSpec.hs
| describe "renderLocator" $ do | ||
| it "should strip non-printable characters such as NUL bytes" $ do | ||
| let locator = Locator "go" "github.com/gin-gonic\NUL/gin" (Just "v1.\t9.1\DEL") | ||
| renderLocator locator `shouldBe` "go+github.com/gin-gonic/gin$v1.9.1" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Test correctly validates non-printable character stripping.
The test case covers the main use case from the PR objectives (NUL bytes in dependency names) and includes multiple control character types (\NUL, \t, \DEL). The implementation correctly verifies these are removed from the rendered locator string.
Optional enhancement: Consider adding test cases for edge cases such as a locator with no non-printable characters (regression safety) or fields that become empty after stripping, though the current coverage is sufficient for the acceptance criteria.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/Srclib/TypesSpec.hs` around lines 19 - 22, The renderLocator test
currently validates the main use case with multiple control characters but lacks
edge case coverage. Consider adding two additional test cases to the
renderLocator describe block: one test verifying that renderLocator correctly
handles a locator with no non-printable characters present (regression safety),
and another test covering the edge case where fields become empty or nearly
empty after stripping non-printable characters. These additions will improve the
robustness of the test suite.
2 participants
Overview
Remove non-printable characters from locators in the
sourceUnitssection of the CLI output. The graphs in the "projects" section still include non-printable characters, as arguably leaving them in is more correct and we don't currently have issues with them being there (mostly because we don't read that output).Acceptance criteria
Locators in source units don't include non-printable characters.
Testing plan
In a new directory, create a pyproject.toml with the following contents:
Run
cabal run fossa -- analyze --output ./your-test-dirand confirm that the locators in the source units don't contain null characters.Risks
Highlight any areas that you're unsure of, want feedback on, or want reviewers to pay particular attention to.
Example: I'm not sure I did X correctly, can reviewers please double-check that for me?
Metrics
Is this change something that can or should be tracked? If so, can we do it today? And how? If its easy, do it
References
CORE-7083
Checklist
docs/.docs/README.msand gave consideration to how discoverable or not my documentation is.Changelog.md. If this PR did not mark a release, I added my changes into an## Unreleasedsection at the top..fossa.ymlorfossa-deps.{json.yml}, I updateddocs/references/files/*.schema.jsonAND I have updated example files used byfossa initcommand. You may also need to update these if you have added/removed new dependency type (e.g.pip) or analysis target type (e.g.poetry).docs/references/subcommands/<subcommand>.md.