resolve #107 IO module out outLn etc

[martinhelmer]

[#107] Implement 'out', 'outLn', 'err' and 'errLn' functions for outputting 'Text' to corresponding handlers

functions added in IO module
refactored existing calls to use new functions

Additional tasks

• Documentation for changes provided/changed
• Tests added
• Updated CHANGELOG.md

[chshersh]

Looks neat 👍🏻

A few suggestions to make the API more convenient, and also some improvements to tests.

[chshersh]

Let's actually make this function work in a polymoprhic MonadIO, so people won't need to write liftIO. And you actually remove liftIO $ ... in all places where you use out, outLn and etc.

Suggested change

	out :: Text -> IO ()
	out :: MonadIO m => Text -> m ()

[chshersh]

I guess this was copy-pasted from another module, so we can update the year 😅

Suggested change

	Copyright : (c) 2022 Dmitrii Kovanikov
	Copyright : (c) 2023 Dmitrii Kovanikov

[chshersh]

I think the hSilence part should be removed:

Suggested change

	checkStdErr = hCapture_ [stderr] . hSilence [stdout]
	checkStdErr = hCapture_ [stderr]

Otherwise, the test it "does not write to stderr " $ checkStdErr (IO.out "TEXT") >>= flip shouldBe "" doesn't really check that the function doesn't output anything to stderr. Because stderr was explicitly silenced.

[martinhelmer]

we need to silence the handle we're not checking. otherwise erroneous behaviour will cause "TEXT" to leak into the console, messing up the test progress report.

I believe the implementation is correct as it is. checkStdErr does not silence stderr. it silences stdout.

i can also confirm that the tests do seem to work; hacking IO.out to print to both stdout and stderr gives ths following result:

   IO
    out
      writes to stdout, no LF  [✔]
      does not write to stderr  [✘]
    outLn
      writes to stdout, LF  [✔]
      does not write to stderr  [✔]
    err
      writes to sterr, no LF  [✔]
      does not write to stdout  [✔]
    errLn
      writes to stderr, LF  [✔]
      does not write to stdout  [✔]

Failures:

  test/Test/Iris/IO.hs:25:83:
  1) Iris.IO.out does not write to stderr
       expected: ""
        but got: "TEXT"

but maybe i'm still missing something...

[chshersh]

I see now. I was confused by silence but I can see that both functions checkStdOut and checkStdErr are used in a pair to test the output to both stdout and stderr. A single test is not enough to make sure that the function doesn't print somewhere else, but two tests together make everything work perfectly 👍🏻

No need to change these helper functions 😌

[chshersh]

You can use shouldReturn from hspec to avoid >>= flip

Suggested change

	it "writes to stdout, no LF " $ checkStdOut (IO.out "TEXT") >>= flip shouldBe "TEXT"
	it "writes to stdout, no LF " $ checkStdOut (IO.out "TEXT") `shouldReturn` "TEXT"

[chshersh]

Looks great! A few minor nitpicks but this is almost done 🏁

[chshersh]

This do seem redundant

Suggested change

	out = do
	liftIO . Text.hPutStr stdout
	out = liftIO . Text.hPutStr stdout

[chshersh]

This should be outLn and not just out.

Ideally, if we could fully test this function, the tests could've caught this error. But I don't see a good way to test it. I was thinking about mocking stdin somehow to provide input. But this looks impossible (or maybe I just don't know how to do this). Tricks like silently allow to capture only stdout and stderr but they don't allow to substitute stdin which is a pity.

A proper test for this function would be to write a separate executable and use some scripting-scenario framework where you can specify input and expected output. But this is a lot of work for this small test, but maybe we'll add it later eventually anyway when things stabilise.

P.S. I remember already writing this comment but I don't see it in the review. Maybe I forgot to submit it 🤔

[martinhelmer]

Have you considered a future improvement which would be "take interactive input from file instead of stdin"? (Useful for batch installation scripts etc)

If we were to support such an improvement we'd need to store the input handle in the environment. And that would also allow for unit testing this part easier.

Thoughts?

[chshersh]

I wrote multiple CLI tools but I never had the need to test in batch mode from a file 🤔

You don't need to mock stdin in such cases because you can always pipe the content of the file into your program:

cat my-commands | my-program

For full E2E testing of CLI tools, one can use an external solution like this one:

• https://github.com/simonmichael/shelltestrunner

I can see how storing stdin in the environment can make testing easier but it can also make the architecture of a CLI app more complicated, confusing and potentially introduce errors when you read not from the environment stdin but from the global one

So I propose just changing IO.out to IO.outLn here and invest if full integration testing later.