Skip to content

BabLoRP: Multi-language support#81

Merged
mircealungu merged 531 commits into
masterfrom
BabLoRP-master
Mar 21, 2026
Merged

BabLoRP: Multi-language support#81
mircealungu merged 531 commits into
masterfrom
BabLoRP-master

Conversation

@lottejd
Copy link
Copy Markdown
Collaborator

@lottejd lottejd commented Mar 19, 2026

Research project & thesis by @BabetteB & @lottejd

BabetteB and others added 30 commits February 5, 2026 16:17
…es that has a fits the parser language mpping
Copy link
Copy Markdown
Contributor

@mircealungu mircealungu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Multi-language support

This is a substantial piece of work with clean architecture (Domain/Application/Infra layers, factory patterns, value objects like RelativePath). ~3,400 lines of tests, good test infrastructure, and solid documentation additions. That said, there are several issues that should be addressed before merging.


Blocking Issues

1. Binary/compiled files committed to the repo
~18 .dll files, a .pdb, Archlens.deps.json, and a binary snapshot are committed under src/python/src/.dotnet/. These are build artifacts that permanently bloat the repo. They should be removed and users should build locally via dotnet publish.

2. Fork-specific references left in

  • archlens.json points to BabLoRP/ArchLens instead of archlens/ArchLens
  • .github/workflows/build.yml hard-codes SonarQube org as "bablorp" and project key "BabLoRP_ArchLens"
  • config.schema.json's $schema URL points to the fork

3. Shell injection risk in cli_interface.py
Lines constructing PowerShell commands use string interpolation from config values + shell=True. A malicious archlens.json with shell metacharacters in project/view names could be exploited. Also, powershell calls are Windows-only and will fail on Linux/macOS.

4. pythonnet loading at import time
load("coreclr") runs unconditionally when cli_interface.py is imported. If .NET runtime isn't installed, the entire CLI breaks — even for Python-only projects.


Bugs

  • UpdateGraphUseCase: Fetches the remote snapshot twice in diff mode. The second call should reuse snapshotGraph.
  • DependencyGraphBuilder.TryClassifyItem: async Task method called without await (suppressed via #pragma). Exceptions are silently swallowed.
  • ConfigManager.MapBaseOptions: Uses baseDir.Split("\\") — Windows-only. Should use Path.GetFileName(baseDir).
  • ProjectDependencyGraph.RemoveDependency: Returns true even when the target was not found in deps.
  • should_run_dotnet(): Logic not (len(...) > 1) means multi-language projects (e.g., .cs + .go) fall through to the Python parser, which doesn't support them — defeating the purpose of multi-language support.
  • GitSnapshotManager.GetLastSavedDependencyGraphAsync: Returns null but signature is non-nullable Task<ProjectDependencyGraph>.
  • Go/Java/Kotlin parsers: StreamReader not in using blocks — file handle leaks on exceptions.
  • JavaDependencyParser: Regex recompiled on every source line (unlike Kotlin parser which does it correctly).
  • GitSnapshotManager.BuildRawUrl: Branch name is not URL-encoded, breaking for branches like feature/foo.

Typos

  • "snaphot" in config schema (should be "snapshot")
  • "completeViewDetph2" in archlens.json (should be "completeViewDepth2")
  • Deserialize error message says "serialise" instead of "deserialise"
  • README: "also have a dedicted" → "also has a dedicated", "reccommend" → "recommend"
  • CONTRIBUTING.md: "is build after" → "is built after"

CI Concerns

  • Lint job does git add -A and auto-pushes — could accidentally stage sensitive files. Should use specific file globs.
  • lint and build jobs run in parallel, so build may test stale code if lint pushes a commit.
  • No Python tests in CI.

Known-Broken Tests Shipped

Several tests are committed with documented failures:

  • Case-insensitive exclusion matching (BIN/bin, .DEV.CS)
  • ExcludedSnapshotItems_ShouldNotBeReportedAsDeleted_ButCurrentlyWillBe — the test name itself admits the bug

What's Done Well

  • Clean architecture with proper Domain/Application/Infra separation
  • RelativePath value object prevents string path confusion
  • MessagePack + Lz4 for snapshot serialization — compact and fast
  • Good test infrastructure (TestFileSystem, TestHttpHandler, parser spies)
  • ~3,400 lines of tests covering parsers, renderers, change detection, graph building
  • RenderGraph intermediate representation cleanly decouples domain from rendering
  • Source-generated regex in PlantUMLRenderer
  • Solid CONTRIBUTING.md and PR template additions

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants