Skip to content

Conversation

@etiktin
Copy link
Contributor

@etiktin etiktin commented Sep 26, 2015

This commit fixes render.clear and resolves #4.

Also removed stream.write('\n'); from render.done, because it adds an extra empty line (the output in render already includes \n, so the caret is already on the next line).

This commit fixes `render.clear` and resolves sindresorhus#4.

Also removed `stream.write('\n');` from `render.done`, because it adds an extra empty line (the output in render already includes `\n`, so the caret is already on the next line).
@sindresorhus
Copy link
Owner

@dthree Thoughts on this?

@dthree dthree mentioned this pull request Oct 1, 2015
@dthree
Copy link

dthree commented Oct 1, 2015

I tested this out in combination with my PR on vorpal-less and all the changes work out fine. Don't see any problems with it.

index.js Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it matters, but seeing as I often use this in a hot loop and some terminals can be slow, I prefer to keep it to one stdout write, as previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original line cleared the previous log and printed the new log. The clearing bit was a duplication of what the clear function does, so I thought it might be better to just call it (keeping with the DRY principle).
The other reason to separate it, is that if clear will ever be broken again you will know it immediately since the output will be messed up.

That said, if you think there's a possible performance issue, I'll change it.

@sindresorhus
Copy link
Owner

👍 when my inline comment is resolved.

This is done to improve performance on slow terminals.
@sindresorhus
Copy link
Owner

Thanks @etiktin :)

@etiktin etiktin deleted the patch-1 branch October 2, 2015 16:41
@etiktin
Copy link
Contributor Author

etiktin commented Oct 2, 2015

You're welcome :)

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.

logUpdate.clear() doesn't work

3 participants