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.

Sign up for GitHub

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

Rst test check messages (fix #17280) #17338

Merged
merged 6 commits into from
Mar 17, 2021
Merged

Rst test check messages (fix #17280) #17338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3a0941c
check warnings and errors in RST test (fix #17280)
a-mr Mar 11, 2021
b251a48
add regression test & improve enumeration warning
a-mr Mar 11, 2021
67a436c
address review comments
a-mr Mar 12, 2021
b29d7f0
Update tests/stdlib/trstgen.nim
a-mr Mar 12, 2021
50a7a4f
fix column offsets properly (almost)
a-mr Mar 13, 2021
2174592
address review comment
a-mr Mar 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions compiler/docgen.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ from std/private/globs import nativeToUnixPath
const
exportSection = skField
docCmdSkip = "skip"
DocColOffset = "## ".len # assuming that a space was added after ##
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


type
TSections = array[TSymKind, Rope]
Expand Down Expand Up @@ -322,8 +323,12 @@ proc genComment(d: PDoc, n: PNode): string =
when false:
# RFC: to preseve newlines in comments, this would work:
comment = comment.replace("\n", "\n\n")
renderRstToOut(d[], parseRst(comment, toFullPath(d.conf, n.info), toLinenumber(n.info),
toColumn(n.info), (var dummy: bool; dummy), d.options, d.conf), result)
renderRstToOut(d[],
parseRst(comment, toFullPath(d.conf, n.info),
toLinenumber(n.info),
toColumn(n.info) + DocColOffset,
(var dummy: bool; dummy), d.options, d.conf),
result)

proc genRecCommentAux(d: PDoc, n: PNode): Rope =
if n == nil: return nil
Expand Down
29 changes: 20 additions & 9 deletions lib/packages/docutils/rst.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@
## .. _Sphinx directives: https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html

import
os, strutils, rstast, algorithm, lists, sequtils
os, strutils, rstast, std/enumutils, algorithm, lists, sequtils,
std/private/miscdollars

type
RstParseOption* = enum ## options for the RST parser
Expand Down Expand Up @@ -477,17 +478,18 @@ type
EParseError* = object of ValueError

const
LineRstInit* = 1 ## Initial line number for standalone RST text
ColRstInit* = 0 ## Initial column number for standalone RST text
## (Nim global reporting adds ColOffset=1)
LineRstInit* = 1 ## Initial line number for standalone RST text
ColRstInit* = 0 ## Initial column number for standalone RST text
## (Nim global reporting adds ColOffset=1)
ColRstOffset* = 1 ## 1: a replica of ColOffset for internal use

template currentTok(p: RstParser): Token = p.tok[p.idx]
template prevTok(p: RstParser): Token = p.tok[p.idx - 1]
template nextTok(p: RstParser): Token = p.tok[p.idx + 1]

proc whichMsgClass*(k: MsgKind): MsgClass =
## returns which message class `k` belongs to.
case ($k)[1]
case k.symbolName[1]
of 'e', 'E': result = mcError
of 'w', 'W': result = mcWarning
of 'h', 'H': result = mcHint
Expand All @@ -497,7 +499,9 @@ proc defaultMsgHandler*(filename: string, line, col: int, msgkind: MsgKind,
arg: string) =
let mc = msgkind.whichMsgClass
let a = $msgkind % arg
let message = "$1($2, $3) $4: $5" % [filename, $line, $col, $mc, a]
var message: string
toLocation(message, filename, line, col + ColRstOffset)
Copy link
Member

@timotheecour timotheecour Mar 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

message.add " $1: $2" % [$mc, a]
if mc == mcError: raise newException(EParseError, message)
else: writeLine(stdout, message)

Expand Down Expand Up @@ -1954,25 +1958,32 @@ proc parseEnumList(p: var RstParser): PRstNode =
if match(p, p.idx, wildcards[w]): break
inc w
assert w < wildcards.len

proc checkAfterNewline(p: RstParser, report: bool): bool =
timotheecour marked this conversation as resolved.
Show resolved Hide resolved
## If no indentation on the next line then parse as a normal paragraph
## according to the RST spec. And report a warning with suggestions
let j = tokenAfterNewline(p, start=p.idx+1)
let requiredIndent = p.tok[p.idx+wildToken[w]].col
if p.tok[j].kind notin {tkIndent, tkEof} and
p.tok[j].col < p.tok[p.idx+wildToken[w]].col and
p.tok[j].col < requiredIndent and
(p.tok[j].col > col or
(p.tok[j].col == col and not match(p, j, wildcards[w]))):
if report:
let n = p.line + p.tok[j].line
let msg = "\n" & """
not enough indentation on line $2
(if it's continuation of enumeration list),
(should be at column $3 if it's a continuation of enum. list),
or no blank line after line $1 (if it should be the next paragraph),
or no escaping \ at the beginning of line $1
(if lines $1..$2 are a normal paragraph, not enum. list)""".
unindent(8)
rstMessage(p, mwRstStyle, msg % [$(n-1), $n])
let c = p.col + requiredIndent + ColRstOffset
rstMessage(p, mwRstStyle, msg % [$(n-1), $n, $c],
p.tok[j].line, p.tok[j].col)
result = false
else:
result = true

if not checkAfterNewline(p, report = true):
return nil
result = newRstNodeA(p, rnEnumList)
Expand Down
8 changes: 4 additions & 4 deletions lib/packages/docutils/rstgen.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1476,7 +1476,8 @@ $content
# ---------- forum ---------------------------------------------------------

proc rstToHtml*(s: string, options: RstParseOptions,
config: StringTableRef): string =
config: StringTableRef,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will be overloaded in the future.

msgHandler: MsgHandler = rst.defaultMsgHandler): string =
## Converts an input rst string into embeddable HTML.
##
## This convenience proc parses any input string using rst markup (it doesn't
Expand All @@ -1503,11 +1504,10 @@ proc rstToHtml*(s: string, options: RstParseOptions,

const filen = "input"
var d: RstGenerator
initRstGenerator(d, outHtml, config, filen, options, myFindFile,
rst.defaultMsgHandler)
initRstGenerator(d, outHtml, config, filen, options, myFindFile, msgHandler)
var dummyHasToc = false
var rst = rstParse(s, filen, line=LineRstInit, column=ColRstInit,
timotheecour marked this conversation as resolved.
Show resolved Hide resolved
dummyHasToc, options)
dummyHasToc, options, myFindFile, msgHandler)
result = ""
renderRstToOut(d, rst, result)

Expand Down
Loading