Fix broken sidebar links for chained class aliases#1675
Fix broken sidebar links for chained class aliases#1675
Conversation
|
🚀 Preview deployment available at: https://7bedae16.rdoc-6cd.pages.dev (commit: de9f6ac) |
There was a problem hiding this comment.
Pull request overview
Fixes broken Darkfish sidebar links for class/module aliases when aliases chain through other aliases (e.g., A = B = C) by ensuring navigation URLs resolve to the underlying real class/module page (since alias pages are not generated).
Changes:
- Make
RDoc::ClassModule#name_for_pathfollow alias chains recursively. - Resolve chained class/module aliases to the real target during
RDoc::ClassModule#update_aliases. - Add a Darkfish generator test intended to reproduce #1664.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/rdoc/code_object/class_module.rb |
Resolve alias chains when computing paths and when updating constant aliases in the store. |
test/rdoc/generator/darkfish_test.rb |
Adds a regression test for sidebar links involving chained aliases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| direct_alias_const = RDoc::Constant.new 'DirectAlias', nil, '' | ||
| direct_alias_const.record_location @top_level | ||
| direct_alias_const.is_alias_for = original | ||
| parent.add_constant direct_alias_const | ||
| parent.update_aliases | ||
|
|
||
| direct_alias = @store.find_class_or_module 'Parent::DirectAlias' | ||
|
|
||
| chained_alias_const = RDoc::Constant.new 'ChainedAlias', nil, '' | ||
| chained_alias_const.record_location @top_level | ||
| chained_alias_const.is_alias_for = direct_alias | ||
| parent.add_constant chained_alias_const | ||
| parent.update_aliases | ||
|
|
There was a problem hiding this comment.
The test setup doesn’t closely match the production code path that triggered #1664: here ChainedAlias is set to alias an already-processed store entry (direct_alias), so it primarily exercises the cm = cm.is_alias_for while ... part, but not the new @store.find_class_or_module(cm.full_name) lookup that’s intended to resolve a chain when const.is_alias_for still points at the pre-update_aliases placeholder object for the intermediate alias.
Consider restructuring to add both alias constants before a single parent.update_aliases (or using Context#add_module_alias to build DirectAlias/ChainedAlias the way the parser does) and ensure ChainedAlias initially aliases the intermediate non-alias object, so this test fails if the store-lookup chain resolution regresses.
lib/rdoc/code_object/class_module.rb
Outdated
| cm = cm.is_alias_for while cm.is_alias_for | ||
|
|
There was a problem hiding this comment.
cm = cm.is_alias_for while cm.is_alias_for calls is_alias_for twice per loop iteration (once for the condition, once for the assignment). Consider rewriting this loop to only evaluate is_alias_for once per iteration for clarity and to avoid redundant calls.
| cm = cm.is_alias_for while cm.is_alias_for | |
| while (next_cm = cm.is_alias_for) | |
| cm = next_cm | |
| end |
When Ruby defines chained aliases like `A = B = C` (e.g., `HTTPRequestURITooLarge = HTTPRequestURITooLong = HTTPURITooLong` in net/http), the sidebar link for the outermost alias pointed to the intermediate alias's HTML file, which is never generated. The root cause: `update_aliases` creates ClassModule objects whose `is_alias_for` references stale intermediate objects from `add_module_alias` that don't know they are aliases. Resolve through the store to follow already-processed aliases back to the real class. Also make `name_for_path` follow the alias chain recursively as a defensive measure. Fixes #1664.
st0012
force-pushed
the
fix-chained-alias-nav-link
branch
from
9e7ccd1 to
de9f6ac
Compare
Summary
HTTPRequestURITooLarge = HTTPRequestURITooLong = HTTPURITooLonginnet/http)update_aliasesby looking up the current store entry to follow already-processed aliases back to the real classname_for_pathfollow the chain recursively as a defensive measureFixes #1664