tables module uses runnableExamples

[ringabout]

No description provided.

[timotheecour]

CI fails:

2021-02-10T04:03:36.7309940Z Test /Users/runner/work/1/s/nimsuggest/tests/tsug_regression.nim
2021-02-10T04:03:37.6800070Z ==== STDIN ======================================
2021-02-10T04:03:37.6800450Z 
2021-02-10T04:03:37.6800860Z Test failed: /Users/runner/work/1/s/nimsuggest/tests/tsug_regression.nim
2021-02-10T04:03:37.6801510Z   Expected:  */lib/pure/collections/tables.nim	374	5	"Returns true *"	100	None
2021-02-10T04:03:37.6802150Z sug	skProc	tables.clear	proc (t: var Table[clear.A, clear.B])	*/lib/pure/collections/tables.nim	567	5	"Resets the table so that it is e
2021-02-10T04:03:37.6803140Z   But got:   /Users/runner/work/1/s/lib/pure/collections/tables.nim	351	5	"Returns true if `key` is in the table `t`.\x0A\x0ASee also:\x0A* `contains proc<#contains,Table[A,B],A>`_ for use with the `in` operator\x0

refs #16401 (comment)

please add dedicated module(s) only imported by nimsuggest tests:
nimsuggest/tests/mimported.nim otherwise we'll end up with nimsuggest tests always failing everytime a change is made to those modules in stdlib. (as you've experienced in your PR)
nimsuggest tests should only rely on its own support files nimsuggest/tests/m*.nim
If for some reason we really want to try nimsuggest on a real codebase (eg compiler/nim.nim), then at least the test should be made more robust to code changes, using for example: [...]

/cc @saem are the stdlib imports import tables, sets, parsecfg in nimsuggest/tests/tsug_regression.nim intentional or can we refactor to avoid stdlib dependency causing issues like this?

[saem]

are the stdlib imports import tables, sets, parsecfg in nimsuggest/tests/tsug_regression.nim intentional or can we refactor to avoid stdlib dependency causing issues like this?

This was a regression test, I think refactoring it to not use stdlib is totally safe. Just need to suss out the essence.

[ringabout]

I can't disable nimsuggest tests, does anyone have some ideas?

[timotheecour]

LGTM, we can re-enable nimsuggest/tests/tsug_regression.nim in followup PR after either updating the test, or preferably, after avoiding referring to stdlib in that test

[dom96]

I like most of these changes, but I don't get the point in moving from the double ` to single. Is it just to be consistent?

As an aside, the markdown headings look weird with Nim's ## comments. Maybe use the multi-line comment syntax? #[ ... ]#?

[timotheecour]

I like most of these changes, but I don't get the point in moving from the double ` to single. Is it just to be consistent?

https://nim-lang.github.io/Nim/contributing.html

In nim sources, prefer single backticks to double backticks since it's simpler and nim doc supports it (even in rst files with nim rst2html).

(besides being simpler/shorter, people are more familiar with markdown than rst)

As an aside, the markdown headings look weird with Nim's ## comments. Maybe use the multi-line comment syntax? #[ ... ]#?

that's to workaround #16990; but multi-line block comment would work too (and in fact I also prefer block comment syntax; the only downside being that some tools won't tell whether something is inside a comment (eg: cat foo.nim | grep bar), but it's an acceptable tradeoff)

[dom96]

I like most of these changes, but I don't get the point in moving from the double ` to single. Is it just to be consistent?

https://nim-lang.github.io/Nim/contributing.html

Ahh, I see you've added this: 4dfb062 :)

I would say that double backticks should be used for two reasons:

• Proper Restructured Text will italize text with single backticks
• Markdown supports both double and single
• This is what Nim's docs have been using for years

If you really want to use single backticks then I'd say there should be a single PR made to change them all at once to avoid noise in PRs that just want to improve documentation.

[timotheecour]

Proper Restructured Text will italize text with single backticks

this doesn't apply to nim sources, for which nim uses its own custom markdown/rst/nim-esque parser. I think most people prefer the simpler form (single backtick).

If you really want to use single backticks then I'd say there should be a single PR made to change them all at once to avoid noise in PRs that just want to improve documentation.

I agree, we can do a PR that replaces all double backticks with single backticks (and noes nothing else) across nim repo (only touching nim, nims files). This can be done after this PR.

[a-mr]

In RST single backticks are reserved for "interpreted text" with a role, absence of role means a default role.

See: https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#interpreted-text

(In original RST there is a directive for switching this default role, .. default-role::: https://docutils.sourceforge.io/docs/ref/rst/directives.html#default-role )

In Nim currently single backticks and double backticks are the same and they mean just monospace text.

I'm OK with allowing single quotes. But why change double->single deliberately?

[timotheecour]

@a-mr you gave me an idea: #17028, so thanks for mentioning default-role!

But why change double->single deliberately?

1 way to do things is better than 2 ways; and as said earlier single backticks is both simpler to read/write in code, and more familiar for vast majority of people.