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

Fix ANSI control sequences in Jupyter notebooks #193

Merged
merged 11 commits into from May 11, 2022

Conversation

nschrader
Copy link
Contributor

@nschrader nschrader commented May 4, 2022

I suggest this fix for #176.

  1. it checks if stdout is a TTY before printing private ANSI control sequences (DECTCEM 1&2)
  2. I also corrected the EL sequence, which was missing the 0 for its canonical form (^[K is a special case of ^[0K)
  3. I added a CR before the EL (which was already partly the case, but cluttered around the code).
  4. some clean-up in the code calling the foregoing functions

Other control sequences seem to work fine in Jupyter. Tests:

  • Jupyter notebook
    Peek 2022-05-05 10-31
  • Terminal
    Peek 2022-05-05 10-32

@pavdmyt
Copy link
Owner

pavdmyt commented May 6, 2022

Hi @nschrader

Thanks for your PR!
I'd suggest to squash your commits and write a meaningful commit message. Also, seems like Travis CI integration has failed in some way, so I have to enable automatic PR testing again.

@pavdmyt
Copy link
Owner

pavdmyt commented May 6, 2022

Running tests manually (make test) on your branch, there are following failures:

    1236 passed
      13 failed
         - tests/test_in_out.py:92 test_hide_show['non-ascii str']
         - tests/test_in_out.py:78 test_write['non-ascii str']
         - tests/test_in_out.py:78 test_write['non-ascii unicode str']
         - tests/test_in_out.py:228 test_write_non_str_objects[foo-foo]
         - tests/test_in_out.py:228 test_write_non_str_objects[obj1-{'cat': 'meow'}]
         - tests/test_in_out.py:92 test_hide_show['non-ascii unicode str']
         - tests/test_in_out.py:228 test_write_non_str_objects[23-23]
         - tests/test_in_out.py:78 test_write['ascii str']
         - tests/test_in_out.py:228 test_write_non_str_objects[obj3-['foo', 'bar', "'", 23]]
         - tests/test_in_out.py:92 test_hide_show['empty']
         - tests/test_in_out.py:92 test_hide_show['ascii str']
         - tests/test_in_out.py:157 test_spinner_hiding_with_context_manager
         - tests/test_in_out.py:181 test_spinner_nested_hiding_with_context_manager
     714 skipped

You have to ensure that tests are working correctly and add a specific test cases for you fix.

@pavdmyt pavdmyt self-assigned this May 6, 2022
@pavdmyt pavdmyt self-requested a review May 6, 2022 09:10
Copy link
Owner

@pavdmyt pavdmyt left a comment

Choose a reason for hiding this comment

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

Please address the above comments.

@nschrader
Copy link
Contributor Author

Thanks for your review, Pavel. Couldn't you simply "squash and merge" this PR on GitHub? So I don't need to force-push anything.

I didn't see the test suite, thanks for the hint! Should be all good now, just needed to adjust for new EL sequence.

@nschrader nschrader requested a review from pavdmyt May 6, 2022 17:37
Copy link
Owner

@pavdmyt pavdmyt left a comment

Choose a reason for hiding this comment

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

Looks good 👍
Just a minor item to clarify.

yaspin/core.py Outdated
@@ -317,7 +307,8 @@ def write(self, text):
# Ensure output is Unicode
assert isinstance(_text, str)

sys.stdout.write("{0}\n".format(_text))
fill = max(0, len(self._text) - len(_text) + 2)
sys.stdout.write("{0}{1}\n".format(_text, " " * fill))
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure I understand this part. Why this extra filling is needed?

Copy link
Owner

@pavdmyt pavdmyt left a comment

Choose a reason for hiding this comment

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

LGTM

@pavdmyt pavdmyt merged commit de08a47 into pavdmyt:master May 11, 2022
@pavdmyt
Copy link
Owner

pavdmyt commented May 11, 2022

@nschrader once again, thanks for your contribution!

I'll include this into the next yaspin release.

@nschrader
Copy link
Contributor Author

@pavdmyt sorry for the mess, but I was testing again this morning and noticed some more problems which Jupyter. Turns out that ^[K isn't actually clearing the line, rather just printing 3 white spaces. That's why ^[0K "fixed" some issues (well, just overrode one more character). I added some more commits, reverting the EL sequence and overriding the current line with white spaces, the only thing that reliably works in Jupyter.

I'd suggest that you revert this PR and I open a new one, so there is a clean trace.

pavdmyt pushed a commit that referenced this pull request Jun 3, 2022
Second try of #193 and fixing #176

- Checking if stdout is a TTY to determine if output is a Jupyter cell or a terminal
- Disabled ANSI control sequence DECTCEM for Jupyter (anyway there is no cursor)
- Replaced ANSI control sequence EL for Jupyter (where it doesn't erase the line, but just prints three white spaces) with some code that simply overrides the entire line with white spaces
- Added patching sys.stdout.isatty to pretend output is a terminal
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.

None yet

2 participants