Allow rdoc-ref to link to non-text files#1376
Conversation
It appears the current behavior of disallowing links to non-text files is deliberate, as it was explicitly added in 3628e19. While the commit message explains the change, it doesn't provide a justification for excluding non-text TopLevels. The issue mentioned in the commit message is also unrelated to the change. It's just as useful to link to a non-text file as it is to link to a text file, so I think it should be allowed.
Cloudflare Preview DeploymentFor Maintainers:🚀 Click here to run the preview deployment workflow This will trigger a Cloudflare Pages preview deployment for this PR. |
|
I agree with the goal but I think we need a bit more changes to achieve that. To test the PR, I added |
The link is correct, the issue is that darkfish doesn't generate the page. Added a commit to fix darkfish, and updated the darkfish tests to test that it is generated. When I was testing earlier versions of this, I was using hanna (as I do for all my projects), which has always generated doc files for non-text files.
The first commit already tests resolution of non-text files: Can you let me know which additional tests you would like? |
st0012
left a comment
st0012
left a comment
There was a problem hiding this comment.
Ok I think I understand the issue here better now:
- Themes can decide whether they want to display non-text files.
hannadisplays them butdarkfishdoesn't - But RDoc's cross referencing feature doesn't allow referencing to non-text files (e.g.
lib/foo.rb) regardless if the theme generates them or not
Is my understanding correct?
If it is:
- I think it's better to revert the 2nd commit as it makes darkfish generate a lot of new pages while most of them are empty. Sorry for the back and forth.
- Can you also update
rdoc-ref's documentation indoc/rdoc/markup_reference.rbto mention how to link to files?
|
|
||
| def page(name) | ||
| @text_files_hash.each_value.find do |file| | ||
| @files_hash.each_value.find do |file| |
jeremyevans
force-pushed
the
rdoc-ref-to-non-text-toplevel
branch
from
d42c1f5 to
45c45d7
Compare
jeremyevans
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
|
I removed the darkfish commit, and added commits to updated the comment and show a usage example. Probably best to squash merge these, but I kept them separate to facilitate review. |
|
🚀 Preview deployment available at: https://5d13ea09.rdoc-6cd.pages.dev (commit: 45c45d7) |
RDoc's cross-reference system auto-links filenames like `array.c` when encountered in documentation text. However, non-text source files are not meant to have their own documentation pages, so these auto-generated links are always broken. Only suppress auto-linking. Explicit `rdoc-ref:` links to non-text files (enabled by #1376) continue to work.
RDoc's cross-reference system auto-links filenames like `array.c` when encountered in documentation text. However, non-text source files are not meant to have their own documentation pages, so these auto-generated links are always broken. Only suppress auto-linking. Explicit `rdoc-ref:` links to non-text files (enabled by #1376) continue to work.
## Summary - RDoc's cross-reference system auto-links filenames like `array.c` when encountered in documentation text, but non-text source files are not meant to have their own documentation pages, so these links are always broken (e.g. https://docs.ruby-lang.org/en/master/extension_rdoc.html#class-library) - Only suppress auto-linking in `ToHtmlCrossref#link`. Explicit `rdoc-ref:` links to non-text files (enabled by #1376) continue to work
3 participants
It appears the current behavior of disallowing links to non-text files is deliberate, as it was explicitly added in 3628e19. While the commit message explains the change, it doesn't provide a justification for excluding non-text TopLevels. The issue mentioned in the commit message is also unrelated to the change.
It's just as useful to link to a non-text file as it is to link to a text file, so I think it should be allowed. I was surprised when it didn't work. I want to use this feature in tilt's documentation.
If there is a reason to disallow it by default, I think this limitation should be documented. I also I think we should add an option to allow it in that case.