Fix docgen snippet numbering #13507

genotrance · 2020-02-26T08:29:38Z

Fix snippet file name - everything was 101 - also print snippet if error
Reduce noise in snippet and example compilation
Reduce specific warnings, hints in koch docs

Araq · 2020-02-26T09:09:35Z

compiler/docgen.nim

@@ -159,6 +159,7 @@ proc getOutFile2(conf: ConfigRef; filename: RelativeFile,
  else:
    result = getOutFile(conf, filename, ext)

+var gIdx = 101


This introduces global state and I don't understand why it's needed.

Araq · 2020-02-26T09:10:21Z

compiler/docgen.nim

      rawMessage(conf, hintExecuting, c2)
      if execShellCmd(c2) != status:
+        echo importStmt & content


No, the compiler doesn't use echo for error reporting and this PR is about making the compiler less noisy, not moreso.

Idea was to print the snippet code that failed, makes it easier to debug the problematic snippet instead of having to search the snippet numerically in source file.

Removed for now.

Araq · 2020-02-26T09:10:51Z

tools/kochdocs.nim

@@ -5,7 +5,7 @@ import os, strutils, osproc, sets
 const
  gaCode* = " --doc.googleAnalytics:UA-48159761-1"
  # --warning[LockLevel]:off pending #13218
-  nimArgs = "--warning[LockLevel]:off --hint[Conf]:off --hint[Path]:off --hint[Processing]:off -d:boot --putenv:nimversion=$#" % system.NimVersion
+  nimArgs = "--warning[LockLevel]:off --warning[Deprecated]:off --warning[UnusedImport]:off --warning[RedefinitionOfLabel]:off --hint[Conf]:off --hint[Path]:off --hint[Processing]:off --hint[XDeclaredButNotUsed]:off -d:boot --putenv:nimversion=$#" % system.NimVersion


This line seems to be the only necessary line to accomplish what your PR title says.

Araq · 2020-02-26T20:34:56Z

compiler/docgen.nim

@@ -211,7 +211,8 @@ proc newDocumentor*(filename: AbsoluteFile; cache: IdentCache; conf: ConfigRef,
      writeFile(outp, importStmt & content)
      let c = if cmd.startsWith("nim "): os.getAppFilename() & cmd.substr(3)
              else: cmd
-      let c2 = c % quoteShell(outp)
+      let subst = if "-r" in c: "-r" else: "$1"
+      let c2 = c.replace(subst, "--verbosity:0 --hints:off " & subst) % quoteShell(outp)


Why not let c2 = (c & "--verbosity:0 --hints:off") % quoteShell(outp)"?

Can't put flags after the filename.

Ok, but let subst = if "-r" in c: "-r" else: "$1" is suspicious. Why does -r lack the $1 part?

Not all tests are run so -r isn't common. But when it is, these flags have to go before it, hence looking for both.

-r seems fragile, eg would trigger for --rangeChecks:on
a more robust solution would identify the position right before the filename in nim cmd -flag1 -flag2 filename arg, ideally by reusing same code compiler uses instead of duplicating logic

Open to ideas but like I said, these params have to go before -r. How about " -r "?

these params have to go before -r

anything between nim binary and filename can go anywhere:

nim c --rangeChecks:on -r main nim c -r --rangeChecks:on main nim --rangeChecks:on -r c main

How about " -r "

not robust to things like:

nim c -r -d:mystr:"foo -r bar" main

(there are more such cases, eg --passC, --passL etc)

sure, these may be rare/edge cases, but I think it's just better to reuse (not re-implement) the logic nim compiler itself uses

how about instead adding --verbosity:0 --hints:off to here:
tools/kochdocs.nim:235:27: commands[i] = nim & " rst2html $# --git.url:$# -o:$# --index:on $#" %
=> see comment below for a better idea

genotrance · 2020-02-29T03:13:30Z

Only freebsd is failing in unrelated test.

timotheecour · 2020-03-01T00:28:01Z

I like the spirit of this PR but not the current implementation.

Some of the warnings are legit and we should fix the code instead of masking the warnings, eg:

/Users/timothee/git_clone/nim/Nim_prs/lib/system/channels.nim(154, 22) Warning: pragma before generic parameter list is deprecated [Deprecated]
    Channel* {.gcsafe.}[TMsg] = RawChannel ## a channel for thread communication
/Users/timothee/git_clone/nim/Nim_prs/lib/system/channels.nim(269, 37) Warning: Number of spaces around '*%' is not consistent [Spacing]
                  cast[pointer](s +% i*% mt.base.size), mt.base, t, mode)

we need to identify which warnings don't make sense for a compiler target, eg: for rst2html RedefinitionOfLabel does not make sense so we should fix it at the compiler level:

compiler/main.nim:

  of "rst2html":
    conf.notes.excl warnRedefinitionOfLabel # similar to issue #13218

here's all the warnings produced:

https://gist.github.com/timotheecour/397948fa2017e147e62e1b4044561060

we can do analysis on this eg:

cat $logsmisc_D/kochdocs.log | grep Warning: | grep -v RedefinitionOfLabel| sort | uniq
cat $logsmisc_D/kochdocs.log | grep Warning: | grep -v RedefinitionOfLabel| sort | uniq | wc -l

=> see #13550 which addresses at the compiler level some of those irrelevant warnings; IMO it's a better approach, eg it doesn't suppress UnusedImport from $nim_prs_D/doc/docgen_sample.nim unlike this PR (which does --warning[UnusedImport]:off), but it does suppress them from things like nimrtl (because of --app:lib); ditto with other flags

genotrance · 2020-03-02T18:14:48Z

I'm fine if this can be fixed in compiler as well as by fixing the actual warnings, there's too many though. Fact is that nim doc warnings are not really attended to, nim doc isn't run in the main CI so there's too many warnings flooding the logs. Nightlies are already a long process.

I can trim this PR to just fixing the snippet numbering.

timotheecour · 2020-03-03T01:20:56Z

I'm fine if this can be fixed in compiler as well as by fixing the actual warnings, there's too many though.

#13550 will fix a lot of these; subsequent PRs to fix remaining ones are welcome

Fact is that nim doc warnings are not really attended to, nim doc isn't run in the main CI so there's too many warnings flooding the logs. Nightlies are already a long process.

out of sight, out of mind; they're much more likely to be attended to if we surface them. Indeed, many tests are only run via nim doc (eg runnableExamples), and simply gagging warnings (including relevant ones) will make it really hard to fix those.

I can trim this PR to just fixing the snippet numbering.

you mean inc(gen.id) ? ya, after reading your whole PR, I think that's the best approach; we can followup after #13550 with more warning slayings as needed.

template since(version, body: untyped) {.dirty, used.} =,

maybe that one might be ok, I don't care either way for that specific one

genotrance · 2020-03-03T14:56:45Z

I've cleaned up this PR to just fix the docgen snippet numbering.

Araq reviewed Feb 26, 2020

View reviewed changes

timotheecour mentioned this pull request Mar 1, 2020

fix #13218: avoid some irrelevant warnings for nim doc,rst2html,--app:lib, + other fixes #13550

Merged

Fix docgen snippet numbering

c8e7f01

genotrance changed the title ~~Reduce noise in koch docs~~ Fix docgen snippet numbering Mar 3, 2020

Araq merged commit f817568 into nim-lang:devel Mar 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix docgen snippet numbering #13507

Fix docgen snippet numbering #13507

genotrance commented Feb 26, 2020

Araq Feb 26, 2020

genotrance Feb 26, 2020

Araq Feb 26, 2020

genotrance Feb 26, 2020

Araq Feb 26, 2020

Araq Feb 26, 2020 •

edited

Loading

genotrance Feb 26, 2020

Araq Feb 26, 2020

genotrance Feb 26, 2020

timotheecour Feb 29, 2020

genotrance Feb 29, 2020

timotheecour Feb 29, 2020

timotheecour Mar 1, 2020 •

edited

Loading

genotrance commented Feb 29, 2020

timotheecour commented Mar 1, 2020 •

edited

Loading

genotrance commented Mar 2, 2020

timotheecour commented Mar 3, 2020

genotrance commented Mar 3, 2020

Fix docgen snippet numbering #13507

Fix docgen snippet numbering #13507

Conversation

genotrance commented Feb 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Araq Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

genotrance commented Feb 29, 2020

timotheecour commented Mar 1, 2020 • edited Loading

here's all the warnings produced:

genotrance commented Mar 2, 2020

timotheecour commented Mar 3, 2020

genotrance commented Mar 3, 2020

Araq Feb 26, 2020 •

edited

Loading

timotheecour Mar 1, 2020 •

edited

Loading

timotheecour commented Mar 1, 2020 •

edited

Loading