Skip to content

Conversation

kaushalmodi
Copy link
Contributor

@kaushalmodi kaushalmodi commented Jun 15, 2018

Also:

  • Move the tests block to the end of the file
  • Fix the older tests
  • Convert styledEcho from a macro to a template, add tests for that.
  • Convert styledWriteLine from a macro to a template, add more tests for that.
  • Add new tests for styledWrite

Fixes #8046.


This commit works, but I don't understand the macro syntax, and would like styledWriteLine to reuse styledWrite that I added.

Let me know if that optimization needs to be made before this can be committed.. or if someone can optimize that after this gets committed.

Also, need to fix these warnings:

terminal.nim(706, 11) Warning: use varargs[untyped] in the macro prototype instead; callsite is deprecated [Deprecated]
terminal.nim(740, 11) Warning: use varargs[untyped] in the macro prototype instead; callsite is deprecated [Deprecated]

Thanks!

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Jun 15, 2018

This was my naive approach for refactoring styledWriteLine, so it of course doesn't work..

macro styledWriteLine*(f: File, args: varargs[typed]): untyped =
  ## Similar to ``writeLine``, but treating terminal style arguments specially.
  ## When some argument is ``Style``, ``set[Style]``, ``ForegroundColor``,
  ## ``BackgroundColor`` or ``TerminalCmd`` then it is not sent directly to
  ## ``f``, but instead corresponding terminal style proc is called.
  ##
  ## Example:
  ##
  ## .. code-block:: nim
  ##
  ##   proc error(msg: string) =
  ##     styledWriteLine(stderr, fgRed, "Error: ", resetStyle, msg)
  ##
  result = newCall(bindSym"styledWrite")
  result.add(f)
  for arg in children(args):
    result.add(arg)
  result.add(newCall(bindSym"write", f, newStrLitNode("\n")))

@kaushalmodi kaushalmodi force-pushed the add-styledWrite branch 2 times, most recently from 6837e5d to 8d9cc55 Compare June 15, 2018 16:33
@Vindaar
Copy link
Contributor

Vindaar commented Jun 15, 2018

You can fix the refactored approach like so:

macro styledWriteLine*(f: File, args: varargs[typed]): untyped =
  # result should be a statement list
  result = newStmtList()
  # create new call with `f` as first argument
  result.add newCall(bindSym"styledWrite", f) 
  for arg in children(args):
    # append all arguments to the `nnkCall` node
    result[^1].add(arg)

  result.add(newCall(bindSym"write", f, newStrLitNode("\n")))

@kaushalmodi
Copy link
Contributor Author

@Vindaar Actually I just converted the 2 unnecessary? macros to templates.. can you check this PR in entirety?

/cc @krux02

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Jun 15, 2018

Earlier when styledWriteLine was a macro, below:

stdout.styledWriteLine(fgRed, "red text ")
stdout.styledWriteLine(fgWhite, bgRed, "white text in red background")
stdout.styledWriteLine(" ordinary text ")
stdout.styledWriteLine(fgGreen, "green text")

resulted in:

image

(Above the underlined "styled text" was blinking and so wasn't caught in that screenshot :))

Now the same test, with styledWriteLine now being a template, and the newline added after the resetAttributes call in styleWrite, results in this:

image

@kaushalmodi kaushalmodi changed the title [WIP] Add styledWrite macro Add styledWrite macro Jun 15, 2018
@kaushalmodi
Copy link
Contributor Author

Except for the "callsite is deprecated" warning that was already there, I am happy with this PR now.

Would be great if someone can review this further.

@krux02
Copy link
Contributor

krux02 commented Jun 15, 2018

just my two cents for the example, you changed stderr.styledWrite to stdout.styledWrite. There is a convention in unix systems, that output on stdout can be further processed by another application if you pipe it into that program, and stderr the actual text the user is supposed to see. processed text in other appications should better not have colors in them. So I prefer if the example of styleWrite remains on stderr. Apart from that I think it's ok.

@kaushalmodi
Copy link
Contributor Author

you changed stderr.styledWrite to stdout.styledWrite

So I prefer if the example of styleWrite remains on stderr.

Just to clarify, the styledWrite wasn't there at all to begin with. It was added in this PR.

As styledWriteLine had an stderr example, I decided to throw in a variety and set the new styledWrite example to have stdout.

There is a convention in unix systems, that output on stdout can be further processed by another application if you pipe it into that program, and stderr the actual text the user is supposed to see.

I completely agree with that. But this example is just about displaying colorful text on stdout.

processed text in other appications should better not have colors in them.

I believe, the application developer should consciously prevent adding colors based on terminal.isatty? (my related blog on that)

@krux02
Copy link
Contributor

krux02 commented Jun 15, 2018

I mean there are things that are obviously still wrong, but they have never been in a better state either. When two threads call styledWriteLine at the same time, the lines can be interleaved. With for example printf, this can't happen. Neither can it happen with echo afaik.

Also:

- Move the tests block to the end of the file
- Fix the older tests
- Add tests for existing styledEcho
- Add new tests for styledWrite

Fixes nim-lang#8046.
This also fixes a bug in the styledWriteLine behavior where the background color
leaked onto the next newline if that command did not end with resetStyle.

Now it is not necessary to end styledWriteLine calls that set BackgroundColor to
end in resetStyle.
@dom96 dom96 merged commit 0da8793 into nim-lang:devel Jun 19, 2018
@dom96
Copy link
Contributor

dom96 commented Jun 19, 2018

Looks good :)

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.

[terminal] Request: styledWrite variant that does not end in a newline
4 participants