allow for explicit exports in generating tags by matthewleon · Pull Request #3205 · purescript/purescript

matthewleon · 2018-01-14T22:57:40Z

Reuses other formats' (HTML, Markdown) machinery for converting PS modules to a friendlier doc module format before generating tags. This eliminates some error-prone, ad-hoc logic for converting declarations directly to tags.

I haven't added any tests onto this branch. I believe that doing so would be made easier if we moved some of the tag generation code from app into Language.Purescript. If people find that appropriate, I'll be happy to add the commits for that onto this branch.

matthewleon · 2018-01-14T23:52:51Z

Another note regarding tests: now that tags use the same intermediate machinery as the other documentation formats, the tests for those formats cover tags as well. We could make tests that are even more "end to end" by testing the string output of the entire tagging workflow, but I feel that the improvements here probably offer a good bang-for-buck compromise.

hdgarrood · 2018-01-15T07:08:57Z

Moving the tag generation code into the library sounds good to me 👍

matthewleon · 2018-01-15T12:44:13Z

Moving the tag generation code into the library sounds good to me

Okay, I'll take a look at this possibility soon.

kritzcreek · 2018-01-20T10:17:00Z

+import Language.PureScript.Docs.Types
+
+tags :: Module -> [(String, Int)]
+tags = map (first T.unpack) . concatMap dtags . modDeclarations


this looks like a nice and pure function, what do you think about adding a few tests to make sure tag generation continues to work as we change the compiler?

I think it's a great idea :). Actually, it's the main reason I proposed moving this code in the first place.

hdgarrood · 2018-01-20T12:41:28Z

  case fmt of
-    Etags -> dumpTags input dumpEtags
-    Ctags -> dumpTags input dumpCtags
+    Etags -> mapM_ putStrLn $ dumpEtags $ zip input ms


I don’t think it’s safe to assume that the modules come out in the same order that they went in. We might need to modify parseAndConvert (instead of doing a zip here) so that it gives you the file path associated with each module.

I'll take a look at this, thanks.

hdgarrood · 2018-01-20T12:42:41Z

+        tagLines = map tagLine $ tags mdl
+        tagLine (name, line) = "\x7f" ++ name ++ "\x01" ++ show line ++ ","
+
+dumpCtags :: [(String, Module)] -> [String]


This isn’t a big deal but if you know why this one sorts but etags doesn’t, it might be useful to leave a comment.

matthewleon · 2018-01-21T17:28:38Z

This is ready for review.

@i-am-tom I recall you being eager to use this functionality. Would you mind trying to run this branch, if you have a moment?

kritzcreek

Code looks good to me, I'll approve but let's wait for positive feedback from Tom before merging.

hdgarrood · 2018-01-21T19:32:45Z

+      let moduleNameToFileMap =
+            M.fromList $ swap . fmap P.getModuleName <$> modules
+          getModuleFile docModule =
+            fromJust $ M.lookup (D.modName docModule) moduleNameToFileMap


I’m not hugely keen on this. I guess this is to reassociate each file path with the relevant module after sending the modules through convertModulesInPackage? Is it worth considering changing convertModulesInPackage so that the output from parseFilesInPackages can just be passed straight in?

I also find it a bit hackish and ugly. Happy to look at your proposal, and see if other options are better as well. Just a clarification: beyond the hackishness of this, is there anything else that you find unpleasant about it that I should be aware of?

(and yes, your guess as to why this is done is correct)

It's mainly the use of the partial function fromJust, but also I think it might be a bit neater to have convertModulesInPackage take responsibility for keeping modules and file paths properly associated with each other so that users of the API exposed by Language.PureScript.Docs don't have to. You already changed parseFilesInPackages in a similar way in this PR and I think it makes sense to have the output of that able to be passed directly in to convertModulesInPackage - currently you can pass the output of the former straight in to the latter.

Makes sense. Thanks for the clarification.

matthewleon · 2018-01-21T22:22:04Z

@hdgarrood I've made a new commit to address your concerns. It is probably not exactly what you were hoping for, but I think it's a reasonable compromise.

My reasoning here:

Actually changing convertModulesInPackage to maintain (path, module) tuples without a Map would involve stringing that change through a large series of functions, some of which are called from multiple sites. It seems to me that it would be a pretty disruptive change.
So instead I've made a new function that uses the same kind of Map-based logic as before, but placed it in the src tree rather than app. This addresses your second concern more than your first; the logic is now easier to reuse.
As for your first concern, regarding the use of fromJust, I've replaced that with use of MonadError's throwError. Admittedly, this still feels a little bit hackish, and might lead to a surprising error message if tag generation ever explodes there. I don't see an obvious better way to do this, though. Please let me know if you can think of something more elegant.

hdgarrood

I suspected that we might end up with something like this, yeah. I agree; the only decent alternative I can think of is your point 1, which would indeed be very disruptive. I think this is a good compromise. Thanks!

i-am-tom · 2018-01-21T23:21:04Z

@kritzcreek I did the tags and the things happen and everything seems to be great :D

matthewleon · 2018-01-21T23:22:58Z

@i-am-tom I'm curious, are you using ctags or etags? Are you using any kind of tagbar plugin, or just doing "jump to definition" type stuff?

i-am-tom · 2018-01-21T23:25:56Z

ctags and just jump-to-type stuff. I am an ever-closer-to-vanilla Vim fellow :)

matthewleon · 2018-01-21T23:28:39Z

Just rebased to make merge easier.

matthewleon · 2018-01-21T23:35:03Z

Hmm, something is killing build on the rebase. My apologies. Will try to fix.

fixes purescript#3204 Reuses other formats' (HTML, Markdown) machinery for converting PS modules to a friendlier doc module format before generating tags. This eliminates some error-prone ad-hoc logic for converting declarations directly to tags.

this makes tag generation more testable

For all tags, use a Map to ensure that filenames and modules correspond correctly. For ctags, add a comment explaining why we sort on tag name. For etags, ensure that they are sorted by module.

Move the logic for maintaining a map between module paths and data from Command.Docs to Language.PureScript.Docs.Convert, allowing other callers to reuse the functionality.

matthewleon · 2018-01-23T00:44:37Z

Okay, this is rebased on the newly fixed master.

hdgarrood · 2018-01-23T11:23:13Z

I’ll just go ahead and merge this, since the Linux and Windows CI builds have all passed already.

hdgarrood · 2018-01-23T11:24:11Z

Thanks very much!

LiamGoodacre added the needs more work label Jan 19, 2018

kritzcreek reviewed Jan 20, 2018

View reviewed changes

hdgarrood reviewed Jan 20, 2018

View reviewed changes

matthewleon force-pushed the fixtags branch from 37fe2be to 0482a85 Compare January 20, 2018 23:53

kritzcreek added status: ready for review and removed needs more work labels Jan 21, 2018

kritzcreek approved these changes Jan 21, 2018

View reviewed changes

hdgarrood suggested changes Jan 21, 2018

View reviewed changes

matthewleon force-pushed the fixtags branch from 9e4eba2 to a8323b0 Compare January 21, 2018 20:15

hdgarrood approved these changes Jan 21, 2018

View reviewed changes

matthewleon force-pushed the fixtags branch from 9a3f054 to 4490954 Compare January 21, 2018 23:28

matthewleon added 5 commits January 22, 2018 19:43

move tag generation into lib

b7795e3

this makes tag generation more testable

ensure proper sorting in tag generation

d1bdf56

For all tags, use a Map to ensure that filenames and modules correspond correctly. For ctags, add a comment explaining why we sort on tag name. For etags, ensure that they are sorted by module.

test tags

53513d7

docs/tags: move logic for linking paths to modules

b89492f

Move the logic for maintaining a map between module paths and data from Command.Docs to Language.PureScript.Docs.Convert, allowing other callers to reuse the functionality.

matthewleon force-pushed the fixtags branch from 4490954 to b89492f Compare January 23, 2018 00:43

hdgarrood merged commit 0798e33 into purescript:master Jan 23, 2018

Conversation

matthewleon commented Jan 14, 2018

Uh oh!

matthewleon commented Jan 14, 2018

Uh oh!

hdgarrood commented Jan 15, 2018

Uh oh!

matthewleon commented Jan 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewleon commented Jan 21, 2018

Uh oh!

kritzcreek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdgarrood Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewleon commented Jan 21, 2018

Uh oh!

hdgarrood left a comment

Choose a reason for hiding this comment

Uh oh!

i-am-tom commented Jan 21, 2018

Uh oh!

matthewleon commented Jan 21, 2018

Uh oh!

i-am-tom commented Jan 21, 2018

Uh oh!

matthewleon commented Jan 21, 2018

Uh oh!

matthewleon commented Jan 21, 2018

Uh oh!

matthewleon commented Jan 23, 2018

Uh oh!

hdgarrood commented Jan 23, 2018

Uh oh!

hdgarrood commented Jan 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hdgarrood Jan 21, 2018 •

edited

Loading