Reexport docs #1770
Reexport docs #1770
Conversation
|
This is what /cc @tfausak |
|
Oh also, fixes #1681 |
|
👍 Looks great! |
|
Ok, this seems to work. I think it's ready for review. I cleaned the history up so that you can probably review them each individually. |
|
Actually, this is not quite there yet. |
| m Module | ||
| expandReExports env ast@(P.Module _ _ mn' _ mexports) mdl = do | ||
| let exports = fromMaybe (err "exports were not elaborated") mexports | ||
| let reExportNames = mapMaybe takeReExport exports |
There was a problem hiding this comment.
One problem with this code is that it can't handle re-exported virtual modules (see also: examples/docs/Virtual.purs: https://github.com/hdgarrood/purescript/blob/reexport-docs/examples/docs/src/Virtual.purs, which, with this code, would cause an internal compiler error).
I want to get a list of all the (real) modules from which values are being re-exported, but I don't want the modules they were originally exported from; I want the modules they were imported from the current module's point of view, that is, I don't want it to follow the whole path of re-exports of re-exports.
This seems to work but of course it chokes on virtual modules. I'm not particularly familiar with this part of the compiler, so I'd appreciate some guidance.
There was a problem hiding this comment.
The information you're after isn't really accessible, as the renaming never takes the intermediate step, it always has multiply-re-exported values pointing at the original export locations.
What's actually the problem with that? You'll have to manually follow the chain of the links through the docs back to the original module if you want to get the definition's comments anyway, so this just takes those steps out of it...
Instead of grouping the re-exported values under a heading for each module, why don't we just lump them all under "Re-exported definitions" in general, and then show the original module info per re-exported member instead? That'd cut down on the number of headers at least, if that's one of the concerns.
There was a problem hiding this comment.
I'm not worried about the number of headings; I arrived at this design because I think it's the best way of presenting this information.
Suppose we have a ByteString type defined in Data.ByteString.Internal, re-exported by Data.ByteString, and then re-exported again by some module A. In that case, I think we would want to write in A's docs that ByteString is re-exported from Data.ByteString. More generally, I think the documentation should present things the same way as they are in the source, because I think that's more likely to make sense to readers of the docs.
The reason for the call to sortModules in this code is to make sure dependencies have already been rendered; this way, we can include the whole declaration for re-exports, including comments, so you wouldn't have to follow the chain back to the source.
There was a problem hiding this comment.
Okidoke. I'm not sure what to suggest for now; the information you need is never constructed. I guess we'll have to change the way the imports/exports are handled to accumulate it somehow, either by reworking the data structures generally or maybe we can tack something in there that allows us to capture the info just for docs.
There was a problem hiding this comment.
Ah ok, maybe this is going to be a little harder then. Save this for after 0.8 perhaps?
There was a problem hiding this comment.
Perhaps, yeah. I'll have a quick look at the "tack on extra info" approach before giving up totally though, as I did that recently to fix the bug where it was warning about some module not existing when it's the value that was actually missing. I added a set of virtual modules to the Env, so perhaps we can change that into a Map and track the imported values there instead or something.
There was a problem hiding this comment.
This is what I'm referring to if you want to take a look yourself?
There was a problem hiding this comment.
Ok, that does sound promising - I'll take a look :)
|
@garyb Your suggestion worked wonderfully! I fixed all the failing tests and cleaned up the history again. I think this is good to go now. |
|
Nice 😄 Although I'm not too sure about the fixed part, seems the build is failing still. |
|
Looks like it's the Line 204 in eb144ec I can look at them again after this is merged. |
|
Ah, no, the docs tests are actually faililng. |
|
Oh, no, it's just printing stdout and stderr out of order: https://travis-ci.org/purescript/purescript/jobs/99953632#L3090-L3097 How do we feel about running |
|
Fixed now, I think. On second thoughts, I'd rather just leave the CI scripts alone. |
|
Looks like it's still failing unfortunately, for the build with disttests enabled. The log is so massive I'm having a hard time seeing what's going wrong though... any ideas? |
|
I think I just worked it out - the new examples, used in the new tests, weren't listed in |
| -- | ||
| , importedVirtualModules :: S.Set ModuleName | ||
| , importedVirtualModules :: M.Map ModuleName ModuleName |
There was a problem hiding this comment.
I'm not sure this is quite right actually, shouldn't it be M.Map ModuleName [ModuleName]? As you can have things like:
import A as X
import B as XThat makes the problem of figuring out where re-exports came from a little awkward again though, so maybe we actually need even more information in the RHS value (something like Imports again with qualified X.foo names pointing to the imported A.foo/B.foo names)
There was a problem hiding this comment.
Oh, yes, of course. Although maybe a [ModuleName] would be sufficient? I think we can look in the other fields of the Imports value for that information?
There was a problem hiding this comment.
Actually, all this code in Docs.Convert is worryingly similar to what's already in Sugar.Names, I think. How would you feel about instead adding a little extra information to the Exports data type, mentioning the first "real" module that declarations come from? Then, in Docs.Convert, we could just walk over the Exports value. We then wouldn't have to look at any DeclarationRef values, or Imports values, or anything.
There was a problem hiding this comment.
That's totally fine by me, it shouldn't complicate the names stuff much, right?
We're already on the second revision of the names stuff and I don't think it's any clearer what is going on than my first attempt at it (although it works much better this time) - I'd like to try yet again to make it a little easier to comprehend and extract this kind of info out of, but that's not something I want to get into right now 😉
|
Half a note to myself, half for the benefit of anyone reading this later: I just had a closer look and I'm pretty sure all the information I need is already in the Env. The Imports value has the module things were imported from on the RHS; in the case of re-exports, it lists the module things were re-exported from. So I can iterate over the Exports, looking in the Imports for each exported thing to find which module I should display it as having come from. I'll also update the comments in Sugar.Names.Env to reflect this. :D |
|
Ok now that I've looked a bit more, we don't quite have enough information as it is now. Take this example: module Docs (module Clash1) where
import Clash1 as Clash1
import Clash2 as Clash2And suppose both Then, the |
|
Wait, we do! I was wrong, the |
Add a variant of Language.PureScript.Sugar.Names.desugarImports which also returns the Env, built up during desugaring imports. We also export Env and related types (Imports, Exports), so that people can get useful information out of the the Env value in this new variant.
* Generalised to work with any Monad which has both MonadIO and MonadError MultipleErrors instances * Remove the useless callback parameter; now, just return the modules and bookmarks instead.
This new information will be necessary for documenting re-exports.
Clearly demarcate each section of the test suite.
This is what cabal seems to expect us to do.
In particular, tests for documentation for re-exports.
* Add a new field in Language.PureScript.Docs.Types.Module for re-exported declarations * Split two child modules out of Language.PureScript.Docs.Convert: Single, which is essentially what Convert previously was, and ReExports, which handles re-exports. * Handle re-exports while converting declarations for documentation (which were previously just ignored). Notably, converting docs can now throw MultipleErrors as a result (although if the code compiles normally, it should always succeed here too). * Update Language.PureScript.Docs.AsMarkdown to include re-exports in the generated documentation. * Export more values from Language.PureScript.Docs.AsMarkdown (because psc-docs needs them now) As an incidental consequence of the above changes, this commit also fixes #1681.
|
Ok, I think this is good to go now. |
|
I'll review this sometime this evening. It'll be great to have it solved at last! |
|
Looks good to me 👍 It's crazy how complicated selective re-exports make things unfortunately (I spent quite a few hours working stuff out related to them for the warnings today), but they are really rather useful. |
|
@paf31 shall I merge this or do you want to review too / have any comments? |
|
@garyb If you're happy with everything, go ahead and merge. Thanks @hdgarrood! |
|
Yeah, thanks @hdgarrood! |
|
🎆 you're welcome. :) |
Addresses #1677.
Changes:
Envis now exported byLanguage.PureScriptdesugarImportswhich also returns theEnv.parseAndDesugarto anyMonadIO m, MonadError P.MultipleErrors mparseAndDesugaralso return theEnv.parseAndDesugar(what was I thinking??)Moduletype inLanguage.PureScript.Docspsc-docs.For now, I just want to see if this passes CI (for some reason, tests aren't working on my machine). Before merging, this still needs: