Skip to content
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

fix #8871 runnableExamples now preserves source code comments, litterals, and all formatting; other bug fix #14439

Merged

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented May 24, 2020

most imporantly:

  • add a lot of regression tests to prevent future regressions

so now both #8871 and #8871 as can be seen in by comparing nim doc --outdir:htmldocs --project nimdoc/testproject/testproject.nim:

for #8871

right before #14441:
image

after this PR:
image

=> we're now preserving formatting exactly (in particular we don't skip comments, change litterals and change indentation etc)

and for #8871

right before #14441:
image

after this PR:
image

=> we're now not skipping doc comments (C) after the 1st runnableExamples (R) so if you have CRCRC you end up with CRCRC instead of CRR; also we're now preserving ordering

@kaushalmodi
Copy link
Contributor

This is awesome! Just yesterday, I noticed that runnableExamples was reformatting my manually set indentation; e.g. this rendered to https://kaushalmodi.github.io/version/#getVersion%2CopenArray%5Bstring%5D%2Cstring

@Araq
Copy link
Member

Araq commented May 25, 2020

Did you consider to re-use nimpretty's approach? Also, is it time to make nimdoc a tool of its own?

@timotheecour timotheecour force-pushed the pr_fix_8871_runnableExamples_verbatim branch 3 times, most recently from d182094 to e38466f Compare May 27, 2020 07:47
@timotheecour
Copy link
Member Author

timotheecour commented May 27, 2020

@Araq PTAL (you can look at generated html for nim doc --project -r nimdoc/testproject/testproject.nim)

Did you consider to re-use nimpretty's approach?

nimpretty doesn't do syntax highlighting so the code I wrote is still needed even with reusing/calling nimpretty.
Future work could nimpretty-fy the source code string (it may or may not be desirable though eg wrt preserving user formatted examples).

This PR got a bit larger than initially because I fixed other issues (see updated note in top post) and added a lot of tests, but the part that's responsible for extracting and rendering runnableExampes (ie fixing #8871) is actually quite concise.

Also, is it time to make nimdoc a tool of its own?

IMO we should completely separate the reflection part (getting postprocessed (after semcheck) AST with module infos, symbols, doc comments properly attached, runnableExamples-as-string, etc) from the rendering part (including running runnableExamples which calls nim-as-binary and doesn't need to depend on compiler sources). All rendering code should be separated from the compiler and would just consume what the 1st step produces (IR, json, PNode, etc see below though).

and here's how it should work IMO:

macros should be powerful enough to enable full reflection and 100% replace docgen

We can have really clean concise nim-doc-as-library by adding just a few magics that would enable full reflection (representing AST after semcheck for each module as 1 NimNode (typed AST)), without even having to import compiler.

This is one of the uses cases I had in mind with #9560 and #8903 and I actually had a working prototype of nim-doc in pure user code using those 2 PR's, that did not need import compiler. It didn't handle many important things like RST parsing, running runnableExamples etc from doc comments, but would be doable especially if we separate rst-logic from compiler and usable in a separate module (stdlib or fusion, probably stdlib since compiler still needs it for now )

Note: this approach invovles 0 serialization (no IR or json needed) since the macros get all they need via a NimNode for each module representing all symbols. It's then possible for user code to then convert these to whatever format needed (json, etc) or to re-do what nim doc does.

other use cases for reflection beyond nim doc

these could be implemented in user code via macros by exposing AST after semantic phase:

  • search tool (eg: list all routines that take an openArray[string] as 1st argument
  • linter (eg: report when result is un-assigned)
  • nimfix (eg: replace noop code like myseq = @[] initializations)
    etc

@timotheecour timotheecour changed the title fix #8871 runnableExamples now preserves source code comments, litterals, and all formatting fix #8871 runnableExamples now preserves source code comments, litterals, and all formatting; other bug fixes May 27, 2020
@timotheecour timotheecour changed the title fix #8871 runnableExamples now preserves source code comments, litterals, and all formatting; other bug fixes fix #8871 runnableExamples now preserves source code comments, litterals, and all formatting; other bug fixe May 27, 2020
@timotheecour timotheecour changed the title fix #8871 runnableExamples now preserves source code comments, litterals, and all formatting; other bug fixe fix #8871 runnableExamples now preserves source code comments, litterals, and all formatting; other bug fix May 27, 2020
@Clyybber
Copy link
Contributor

@timotheecour Love the idea of docgen as a macro.
Maybe it can be done without additional magics via this approach:

import macros

macro genDocs(e: typed) =
  echo treeRepr e

genDocs:
  include moduleWeWantToGenerateDocsFor

@Araq
Copy link
Member

Araq commented May 27, 2020

macros should be powerful enough to enable full reflection and 100% replace docgen

I completely disagree. This idea that we can never ever reduce an AST because it might be rendered back to ascii somewhere did cost us years of implementation effort, keeps costing us effort and makes writing macros more complex. What's the benefit? A good docgen is still as hard to implement as it was before, if not harder and you'll put pressure on the Nim VM.

@kaushalmodi
Copy link
Contributor

@timotheecour Referring to your comment in #14473 (comment),

this should be fixed in #14439 (I just tried), can you confirm on your end and also for your project that #14439 works?

Yes, I just confirmed that this PR fixes the minimal example in #14473, as well as in my real project.

Can you update a test case in this PR to also import std/[sequtils] and use one of the exported symbols from that in the test case to match the test case in #14473 ?

Thanks!

@timotheecour
Copy link
Member Author

Can you update a test case in this PR to also import std/[sequtils] and use one of the exported symbols from that in the test case to match the test case in #14473 ?

done. (see also: nim-lang/RFCs#231)

@timotheecour
Copy link
Member Author

timotheecour commented May 28, 2020

friendly ping @Araq because this fixes a very recent regression that I've introduced in #14441 that prevents some runnableExamples from running; let me know if you instead want me

compiler/docgen.nim Outdated Show resolved Hide resolved
compiler/renderverbatim.nim Outdated Show resolved Hide resolved
compiler/renderverbatim.nim Outdated Show resolved Hide resolved
@timotheecour timotheecour force-pushed the pr_fix_8871_runnableExamples_verbatim branch from afedbaa to 0811b41 Compare May 28, 2020 16:43
@timotheecour
Copy link
Member Author

timotheecour commented May 28, 2020

@Araq PTAL; all points addressed and found I more corner case that i fixed (regular comments before the 1st node inside a runnableExamples are now correctly handled, ie appear as part of the doc comment)

in reply to #14439 (comment)

What's wrong with bodyPos?

I added some explanation inside PR code; whether it's a bug or not I'm not sure, but it seems inconsistent that you have:

  proc someType*(): int =
    ## foo
    result = 3
=> this is weird
  result =
    ## foo
    3;

  proc someType*(): int =
    ## foo
    3
=> this is fine
  ## foo
  result = 3;

IMO the sane behavior is for transf to transform the 1st case into the following instead:

## foo
result = 3;

This has very strong echoes of #14420 (comment)

EDIT: simplified further in #14701

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants