Fix std/random bug (#204)

[sent44]

• Remove OpenSystemsLab/tempfile.nim and replace with build-in std/tempfiles Without randomize() that can be found in that package, the random result in user code will be consistent
• Remove low level stuff and replace with build-in fusion/ioutils As it might break in newer Windows update
• Add ability to name the temp file reuse the same file every time I don't know if this statement mean that you can give name the temp file and if it is already exist then open or temp file name is generate and you can use old file in some way, so I implement the first one. Should not break exist code (Didn't test against the main package)

This should not need to modify docs as it is internal stuff hide be hide nbCode fixed #204

Test

import capture

var s: string

var msg = "hello"
echo msg & "1"
captureStdout(s):
    echo msg & "2"
    msg = "ciao"
echo msg & "3"

assert s == "hello2\n"

import random

echo rand(100) # result: 3
captureStdout(s, "my_random_tmp"):
    echo rand(100) # result: 89
    discard
# echo rand(100)
echo "s=", s

captureStdout(s, "my_random_tmp"):
    echo rand(100) # result: 18
echo "s=", s

captureStdout(s):
    echo "echo"
echo "s=", s

Expect result

hello1
ciao3
3
s= (lots of 89 and 18 depend on how many time you run)

s= (lots of 89 and 18 depend on how many time you run)

s=echo

[sent44]

I am going to investigate the error for the time begin Temporary close

[sent44]

In addition to that you will also have to add fusion as a dependency in the nimib.nimble file.

Isn't fusion already part of semi standard library that already come compiler? Like experimental features So I should OpenSystemsLab/tempfile.nim dependency from nimib.nimble instead

Edit: Oh https://forum.nim-lang.org/t/7608

[HugoGranstrom]

In addition to that you will also have to add fusion as a dependency in the nimib.nimble file.

Isn't fusion already part of semi standard library that already come compiler? Like experimental features So I should OpenSystemsLab/tempfile.nim dependency from nimib.nimble instead

Edit: Oh https://forum.nim-lang.org/t/7608

Yeah, fusion was planned to function that way but now it's mostly abandoned. It does have a few useful libraries though. And yes, you should remove the old OpenSystemsLab/tempfile.nim dependency from the nimble file.

[HugoGranstrom]

All the tests are passing now, so we have solved part of the issue :partying_face: Now we have a new error though, redefinition of decode :/

[sent44]

Now we have a new error though, redefinition of decode :/

I have no idea ._., might need to check with expandMacro to see why somehow it is redefine I am going to sleep now

[HugoGranstrom]

I have no idea ._., might need to check with expandMacro to see why somehow it is redefine I am going to sleep now

Yeah, I'll investigate it. Sweet dream :sleeping:

[HugoGranstrom]

I have managed to reduce it to a minimal example, and it's a compiler problem

template foo(x: string, body: untyped) =
  echo x
  body

template foo(body) =
  foo("Goodbye", body)

foo("Hello"): # works
  proc f() = discard
  echo "World"

foo: # doesn't work
  proc f() = discard
  echo "World"

The problem is that we have an overload of captureStdout, and the compiler needs to check which one it is that we want to call. So it must type-check all the arguments (even if one of them is untyped). And my guess is that in that type-checking it mistakenly saves the proc decode and when we actually define it later it will falsely complain about it already being defined.

I will open an issue on Nim's issue tracker, but the workaround for us would be to don't have two overloads and instead just keep one of them (with or without filename).

[HugoGranstrom]

It seems like this has been discussed here: https://github.com/nim-lang/RFCs/issues/402 And it basically won't be fixed... So we have to do a workaround, and my vote is on removing the filename argument as I simply don't see any use for it. And if we want to reuse a file in the future we will have to refactor this code either way, so we'll deal with that when it comes instead. Your opinion?

[HugoGranstrom]

Also, you will need to merge our main branch into your main-1 branch as I just fixed a piece of code in the docs that didn't compile.

[pietroppeter]

Great work @sent44, very well done and thanks! And excellent analysis by @HugoGranstrom of this weird issue. I would agree we can skip the version with filename.

[HugoGranstrom]

Looks good to me and the CI now. Thanks again for contributing to nimib 😁 I'll merge this in a moment

[pietroppeter]

note that this also closes the older issue of #72. And reading that makes me realise that this fix "breaks" nimib for nim < 1.6. Which is fine, I think. An option would have been to keep older code under when (NimMajor, NimMinor) < (1, 6) but it is probably not worth it at the moment (and we could do it easily later in case there is a need). What is important we do not miss is that in the changelog for next release (3.10?) we mention this breaking change. And also I think this is the only recent change that would warrant a release, right? (the other commits if I am not wrong are improvements/changes for documentation, not in the library).

[HugoGranstrom]

Oh, had totally missed that :O But I agree, 1.4.x is old by now. I think we should keep 1.6.x working as long as possible though as 2.0 brings some breaking changes that could break people's old articles using old versions of libraries.

What is important we do not miss is that in the changelog for next release (3.10?) we mention this breaking change. And also I think this is the only recent change that would warrant a release, right?

Correct, next release is 3.10. These two commits are not included in the latest release either:1 2