feat(mapper): add Django route mapping#54
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f5e591f66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (previous !== undefined && /[A-Za-z0-9_.]/u.test(previous)) { | ||
| return null; |
There was a problem hiding this comment.
The identifier-boundary check in djangoRouteHelperAt misses _ as a valid preceding identifier character, so calls like custom_path('x/', view) inside urlpatterns are parsed from the path( substring and incorrectly emitted as Django routes. This creates false-positive route features (wrong ownership/tests/trust boundaries) in Django projects that use helper functions with names ending in _path, and the issue is introduced by the new Django mapper logic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for calling this out. I checked this path and the matcher already treats _ as part of the preceding identifier, so custom_path(...) is skipped instead of being parsed from the inner path substring.
I added an explicit regression test for this case in src/mapper.test.ts to make that behavior clear and keep it covered going forward. Focused mapper tests, typecheck, and lint are passing.
# Conflicts: # docs/index.md # docs/quickstart.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d8bbc0597
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function djangoRouteHelperAt(source: string, index: number): string | null { | ||
| const previous = source[index - 1]; | ||
| if (previous !== undefined && /[A-Za-z0-9_.]/u.test(previous)) { |
There was a problem hiding this comment.
The boundary check in djangoRouteHelperAt still allows matching path( inside larger identifiers because _ is not included in the preceding-character class, so custom_path('x/', view) is parsed as a Django path(...) call and produces a false route feature. This creates incorrect ownership/tests/trust-boundary mapping in projects that use helper names ending in _path. Fresh evidence in this commit is that the guard regex is /[A-Za-z0-9_.]/, which omits _.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I rechecked this against the merged code and the current test coverage. The guard is /[A-Za-z0-9_.]/u, so _ is already part of the preceding-character class. For custom_path(...), the scanner reaches the p in path, sees _ as the previous character, and returns null rather than treating it as a Django path(...) helper.
This is also covered by the mapper fixture now: it includes custom_path('custom/', views.custom) and asserts /custom/ is not emitted as a Django route. I do not think there is a code change needed for this one.
2 participants
Closes #53
Summary
urls.pyroute mapping forpath,re_path, and legacyurlinclude("app.urls")as a URL group without recursively expanding imported configsurlpatternsTesting
./node_modules/.bin/vitest run src/mapper.test.ts./node_modules/.bin/tsc -p tsconfig.json --noEmit./node_modules/.bin/oxlint . --config oxlint.json./node_modules/.bin/vitest run