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

runnableExamples now run at module level #11732

Merged
merged 6 commits into from
Jul 22, 2019

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jul 14, 2019

this PR allows runnableExamples to generate code at module scope, which is needed in some cases where code inside a block would not work. The option is opt-in so no existing code is affected.

See tests/magics/trunnableexamples.nim for unittests, it also shows some examples where module scope is needed

@Araq
Copy link
Member

Araq commented Jul 15, 2019

But we don't support a combination of default values and untyped elsewhere. Why does it have to be opt-in?

@timotheecour
Copy link
Member Author

timotheecour commented Jul 15, 2019

Why does it have to be opt-in?

I pushed commits to make it opt-out, so the default is now topLevel=true, after checking that:

  • only 1 test was broken: lib/pure/uri.nim (caused by [low priority] bad error message for variable name clashing with module name timotheecour/Nim#17 which I didn't bother filing as a bug in main nim repo since it's rather minor), so hopefuly won't break too much code in the wild either
  • the other concern i had was performance of block vs module scope, but after some quick tests it seems the main performance drop is from function scope to (module scope or block scope), so topLevel=true/false wouldn't make a difference

We still need a way to opt out (ie allow topLevel=false) because there are legitimate things to test that can't use module scope (see tests/magics/trunnableexamples.nim)

But we don't support a combination of default values and untyped elsewhere

true, that's what I was seeking in optional params before untyped body - Nim forum, but looks like an implementation issue that can be fixed in the compiler; unless I'm missing something there's no ambiguity in supporting that.
In any case it doesn't really affect runnableExamples as it's a weird magic implemented in docgen, so I can't think of anything that would break even if https://forum.nim-lang.org/t/4970 never gets fixed

@Araq
Copy link
Member

Araq commented Jul 17, 2019

I don't understand the problem this fixes. runnableExamples is about providing clean, easy snippets, even a when defined(posix): ... in there is usually a bad idea. Is is checked by the tooling that the examples are "runnable" aka do compile so they can't bitrot. If {.codeReordering.} doesn't work within runnableExamples, so be it. You shouldn't use it anyway, runnableExamples is for documentation, not for writing tests.

@zah
Copy link
Member

zah commented Jul 17, 2019

I second the request to implement the support for optional params before untyped body.

Actually, I've implemented quite an elaborate hack to simulate it in our eth-p2p library here:
https://github.com/status-im/nim-eth/blob/master/doc/p2p.md#requestresponse-pairs

@krux02
Copy link
Contributor

krux02 commented Jul 17, 2019

topLevel should not be an argument for runnableExamples. It is just not necessary. If you need a scope, just create one in the example. Create a proc foo, or a template, or a block. Whatever you need.

@zah I agree that optional parameters before a code block are neat. But that should be done in a proper RFC. We can't continue to patch in everything in the language that we think is useful at that moment without properly checking that it won't cause conflicts with other features of the language.

@krux02 krux02 closed this Jul 17, 2019
@timotheecour
Copy link
Member Author

timotheecour commented Jul 17, 2019

topLevel should not be an argument for runnableExamples. It is just not necessary. If you need a scope, just create one in the example. Create a proc foo, or a template, or a block. Whatever you need.

yes, you can add a scope, but you can't remove the one the compiler automatically adds to each runnnableExamples, making it impossible to create runnableExamples for anything that requires moduel scope, see tests/magics/trunnableexamples.nim for some such examples; so i can update the PR to just make runnableExamples imply topLevel=true as the new default, and if user needs scope access, he can add a block

@Araq Araq reopened this Jul 19, 2019
@Araq
Copy link
Member

Araq commented Jul 19, 2019

Just to clarify it further: "fixing" runnableExample so that it supports more features is fine if the patch it not unreasonably large to accomplish this, but don't add yet-another optional parameter to it. You never get a reliable product out of a mess of configuration options, the combinatorial explosion always wins.

@timotheecour
Copy link
Member Author

done, PTAL, runnableExamples now just has semantics of topLevel=true, without option; if user needs he can just simply nest under a block

@Araq Araq merged commit 8c93c69 into nim-lang:devel Jul 22, 2019
@timotheecour timotheecour deleted the pr_runnableExamples_toplevel branch July 22, 2019 17:02
@timotheecour timotheecour changed the title allow runnableExamples(topLevel=true) runnableExamples now run at module level May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants