-
Notifications
You must be signed in to change notification settings - Fork 480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Search harder for cross-module references #1352
base: master
Are you sure you want to change the base?
Conversation
Update, I'm not so sure the problem I thought I saw really is a problem. With this PR, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I understand: this fixes the case where you're referring to a binding that is defined in a module that is not being using
d in make.jl
, but is loaded as a dependency of something and its source code lives in the current repository (i.e. the multi-package repo thing)? And without this patch we can't resolve such at-ref links, because they are not available in Main
?
If I understand this correctly, then yes, I think it's a good idea to have this. I don't think this necessarily conflicts with #1351, but it is probably the better solution for the SnoopCompile use case.
Regarding tests: I think it should be easy to add something to the test/examples
builds that test this code path.
I think it's probably also relevant to the following case: module That's what I've tried to add in my test. There are two senses of "works": do the tests pass according to |
Using throw() like this attaches the wrong backtrace to the exception. Should be rethrow(), but you can only have that in catch blocks. However, in this case, a simple try-finally actually suffices, no need to do anything more complicated. (cherry picked from commit 16bd652)
First, sorry for not getting back to this earlier. When I build
That is also failing the CI. However, it looks to be a heisenbug.. sometimes it doesn't throw that error, which very interesting...
Regarding the tests:
That's how far I got at the moment, I can take another look later in the week. |
Ok, I just saw that this was added here. Passing the submodules by hand to Documenter.jl/src/Documents.jl Lines 349 to 352 in 8226880
|
This provides a partial fix to #688, specifically for the case where we are building docs for multiple modules and some of them may not know about each other. As a partial fix I prefer it to #1351, though for cases this doesn't cover that may have merit too.
This is a WIP because (1) it's missing tests, and (2) while it solves the issue during the
CrossReferences
phase,HTMLWriter
still has an issue:I'm not entirely sure about the internal data structures here. I'm guessing I need something like
Documenter.jl/src/CrossReferences.jl
Lines 153 to 157 in 6b3efa6
@snoopr
is defined and documented inSnoopCompileCore
, butSnoopCompile
has ausing SnoopCompileCore
in it so@snoopr
is visible from inside bothSnoopCompileCore
andSnoopCompile
.)