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

Use alternative to ANSI control sequences in Jupyter notebooks #195

Merged
merged 25 commits into from Jun 3, 2022

Conversation

nschrader
Copy link
Contributor

@nschrader nschrader commented May 11, 2022

Second try of #193 and fixing #176

  1. checking if stdout is a TTY to determine if output is a Jupyter cell or a terminal
  2. disabled ANSI control sequence DECTCEM for Jupyter (anyway there is no cursor)
  3. 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
  4. added a CR before the EL (which was already partly the case, but cluttered around the code)
  5. cleaned up in the code calling the foregoing functions
  6. added patching sys.stdout.isatty to pretend output is a terminal

Terminal:
Peek 2022-05-12 13-21

Jupyter:
Peek 2022-05-12 13-22

Comment on lines +23 to +29
def test_color(monkeypatch, color_test_cases):
color, expected = color_test_cases
# ``None`` and ``""`` are skipped
if not color:
pytest.skip("{0} - unsupported case".format(repr(color)))

monkeypatch.setattr(sys.stdout, "isatty", lambda: True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe there is an easier way to do this over and over?

Copy link
Owner

Choose a reason for hiding this comment

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

It is possible to make this patch at the module level, like this:

@pytest.fixture(autouse=True)
def isatty_true(monkeypatch):
    monkeypatch.setattr(sys.stdout, "isatty", lambda: True)

From the docs: https://docs.pytest.org/en/6.2.x/monkeypatch.html#global-patch-example-preventing-requests-from-remote-operations

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.

Few comments; Also, code conflict should be resolved.

tests/conftest.py Outdated Show resolved Hide resolved
Comment on lines +23 to +29
def test_color(monkeypatch, color_test_cases):
color, expected = color_test_cases
# ``None`` and ``""`` are skipped
if not color:
pytest.skip("{0} - unsupported case".format(repr(color)))

monkeypatch.setattr(sys.stdout, "isatty", lambda: True)
Copy link
Owner

Choose a reason for hiding this comment

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

It is possible to make this patch at the module level, like this:

@pytest.fixture(autouse=True)
def isatty_true(monkeypatch):
    monkeypatch.setattr(sys.stdout, "isatty", lambda: True)

From the docs: https://docs.pytest.org/en/6.2.x/monkeypatch.html#global-patch-example-preventing-requests-from-remote-operations

tests/conftest.py Outdated Show resolved Hide resolved
yaspin/core.py Show resolved Hide resolved
yaspin/core.py Show resolved Hide resolved
@pavdmyt pavdmyt self-assigned this May 15, 2022
@pavdmyt pavdmyt added the conflicts code conflict should be resolved label May 15, 2022
@@ -13,7 +13,7 @@ pypi_pwd := $(shell grep password ~/.pypirc | awk -F"= " '{ print $$2 }')

flake:
@echo "$(OK_COLOR)==> Linting code ...$(NO_COLOR)"
@poetry run flake8 --ignore=F821,E501 .
@poetry run flake8 --ignore=F821,E501,W503 .
Copy link
Contributor Author

Choose a reason for hiding this comment

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

W504 and W503 are contradictory

Comment on lines +103 to +105
@pytest.fixture(autouse=True)
def isatty_true(monkeypatch):
monkeypatch.setattr(sys.stdout, "isatty", lambda: True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, this doesn't correctly patch capsys so more monkeypatches are needed within test cases that use capsys. Do you have another idea?

However, this works fine for a bunch of tests not using capsys that would otherwise raise a warning. 👍

@pavdmyt pavdmyt removed the conflicts code conflict should be resolved label May 29, 2022
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.

Few comments; sorry for such a delay in reviews, having very busy weeks right now.

tests/test_attrs.py Show resolved Hide resolved
tests/test_in_out.py Outdated Show resolved Hide resolved
tests/test_in_out.py Outdated Show resolved Hide resolved
yaspin/core.py Outdated Show resolved Hide resolved
yaspin/core.py Outdated Show resolved Hide resolved
@nschrader nschrader requested a review from pavdmyt May 31, 2022 09:10
@nschrader
Copy link
Contributor Author

@pavdmyt no worries for the delay. I hope my changes fit your requests :)

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.

One minor issue, otherwise LGTM 👍

yaspin/core.py Show resolved Hide resolved
@pavdmyt pavdmyt merged commit 6e96e83 into pavdmyt:master Jun 3, 2022
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