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? |
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?
| @@ -723,7 +723,7 @@ def modules_hash | |||
| # Returns the RDoc::TopLevel that is a text file and has the given +name+ | |||
|
|
|||
| def page(name) | |||
| @text_files_hash.each_value.find do |file| | |||
| @files_hash.each_value.find do |file| | |||
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.