Rst test check messages (fix #17280)

[a-mr]

Check warnings and errors in RST test. Fixes #17280 using symbolName implemented in #17281.

Depends on #17257 since printing of line numbers / columns were changed there. So review only the latest commit: f9c427a

[timotheecour]

use toLocation std/private/miscdollars
rationale: DRY, and enables #690 (comment)

[a-mr]

fixed though it's weird that toLocation omits column if it's 0. So I had to change starting column to 1, which would be worth doing anyway.

[timotheecour]

i wrote it this way to avoid spurious file.nim(1, 0) messages when column is not generated in the 1st place (eg depending on nim's flags)

IMO it's fine because valid columns are assumed to be >= 1

[timotheecour]

use unittest.check or unittest.require, don't roll your own (and assertEqual has the multiple template evaluation bug)

[a-mr]

Well, I switched to check, OK.

However for code

    check(warnings8[] == @["input(4, 0) Warning: unknown substitution " &
            "\'citation-som\'h"])

it seems very verbose:

    /home/amakarov/tmp/Nim/tests/stdlib/trstgen.nim(941, 22): Check failed: warnings8[] ==
    @["input(4, 0) Warning: unknown substitution " & "\'citation-som\'h"]
    warnings8[] was @["input(4, 0) Warning: unknown substitution \'citation-som\'"]
    @["input(4, 0) Warning: unknown substitution " & "\'citation-som\'h"] was @["input(4, 0) Warning: unknown substitution \'citation-som\'h"]

The constant in @[...] is repeated 3 times. Do you think check had better be improved?

[timotheecour]

there are 2 things going on here:

• you're using & which prevent's check's eliding of was msg on lhs
• check's eliding doesn't work for more complex literals like seq, see unittest.check error msg shows was on litteral-like expressions timotheecour/Nim#650

but just assume this will eventually be fixed, and use the shorter code as you did here (it only matters when things go wrong anyway, and just hurts a bit debug experience so it's fine)

[timotheecour]

instead of having to bump up a counter, use blocks so each test is independent in terms of variables in scope (makes it easier to move them around, add some without messing re-ordering, etc)

[a-mr]

Agree. I suggest to do a follow-up PR for trstgen.nim:

• split to separate tests as you said
• use dedent in this test everywhere

wdyt?

[timotheecour]

yes, in a different PR is ok, to make progress faster and reviewing easier :)

[timotheecour]

optional: why raise only to catch later? is this to stop on 1st error? can't we instead:

• either keep track of number of errors seen here and not overwrite error[] after it's been written to once
• or add an option roStopOnFirstError

[a-mr]

yes, just to stop on 1st error.

We can do all of that but there is little sense. In the 1st commit all errors were checked but I saw that they add little value to 1st error. Check for number of errors is better but that would be difficult to develop: if you broke tests around this number then you don't know immediately what those errors were :-)

[timotheecour]

what i meant was keeping track of number of errors so you only set error[] for just the 1st emitted error, and then ignoring subsequent ones. but ok, it's a minor detail.

[a-mr]

Also observed that columns in nim doc (on *.nim files) are shifted by 3.

It appears that a start column of doc comment ## is passed. So I added "## ".len to get the correct column in a previous commit.

There is a little problem here: automatic cropping logic determines number of spaces after ##, which may not be 1, and AFAIK this number is not saved anywhere. So if anyone chooses to not follow the convention 1 space after ## then he'll get a shifted number of columns again.

[timotheecour]

in reply to

Also observed that columns in nim doc (on *.nim files) are shifted by 3.

(i prefer code comments to maintain threading)
take a look at https://gist.github.com/timotheecour/e53f06241914fa1e20a40f631edfae10

run nim doc -d:case1 main (and -d:case2, -d:case3)

before this PR, line and column number is bad
after this PR, line and column number is also bad (maybe a bit worse?)

IMO we should report line and column numbers wrt source file, and not offset it by "## ".len; unless I'm missing something it should be both simpler and more robust.

But I suggest merging this PR to make progress and addressing this inconsistent line/col numbering in followup PR.

[a-mr]

the case1 is just incorrectly adjusted line in parseHeadline. We can fix it with an appropriate test now.

The multiline case2 and case3 are more difficult, some investigation will be needed.

[timotheecour]

in future PR we can add an outplace version of toLocation to make life simpler in here and elsewhere, useful wherever performance is not critical (like here). Or you could also use sugar.dup

[timotheecour]

instead of specifying line, col in all callers (and doing so incorrectly, see below with (1,1) instead of (LineRstInit,ColRstInit) which is (1,0), how about add an overload of rstParse:

proc rstParse*(text, filename: string, hasToc: var bool, options: RstParseOptions, findFile: FindFileHandler = nil, msgHandler: MsgHandler = nil): PRstNode =
  rstParse(line=LineRstInit,column=ColRstInit,...)

will simplify and make these correct:

compiler/docgen.nim:161:12:  result = rstParse(text, filename, line, column, hasToc, rstOptions,
lib/packages/docutils/rst.nim:2593:6:proc rstParse*(text, filename: string,
lib/packages/docutils/rstgen.nim:1509:13:  var rst = rstParse(s, filen, line=LineRstInit, column=ColRstInit,
lib/packages/docutils/rstgen.nim:1523:7:      rstParse(rstSource, "", line=LineRstInit, column=ColRstInit,
tests/stdlib/trstgen.nim:425:30:    rstGenera.renderRstToOut(rstParse(input10, "", 1, 1, option, {}), output10)
tests/stdlib/trstgen.nim:451:32:    rstGenera11.renderRstToOut(rstParse(input11, "", 1, 1, option11, {}), output11)
tests/stdlib/trstgen.nim:480:32:    rstGenera12.renderRstToOut(rstParse(input12, "", 1, 1, option12, {roSupportMarkdown}), output12)
tests/stdlib/trstgen.nim:577:30:    rstGenera.renderRstToOut(rstParse(input1, "", 1, 1, option, {}), output1)

can be done in cleanup PR though

[a-mr]

ok, will be overloaded in the future.

[a-mr]

@Araq @narimiran

Pure RST part should be already correct here.

The problem with columns in genComment is not fixed correctly but the proper fix is hard since it would require to modify the lexer: currently TLineInfo does not preserve columns, it contains only beginning column of ## or ##[, not column of the doc comment itself:

##[ Doc comment
^   ^
|   |
cur |
    should be

I tried to put the line and column of Doc comment itself to n.info but immediately got a problem with nimpretty.

[timotheecour]

@a-mr

The problem with columns in genComment is not fixed correctly

let's followup here: timotheecour#658