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 selected syntax style for tracebacks and improve ANSI color codes support #608

Merged
merged 16 commits into from
Aug 12, 2024

Conversation

jsbautista
Copy link
Contributor

@jsbautista jsbautista commented Jun 4, 2024

Fixed traceback higlight, making use of the VerboseTB hack and adding a color homologation to expand the range of colors used by QtConsole
Before:
BeforeQtConsole
BeforeQtConsolea (online-video-cutter com)

After:

AfterQtConsole
AfterQtConsolea

related issues
spyder-ide/spyder#4891

@jsbautista
Copy link
Contributor Author

@dalthviz

@dalthviz dalthviz changed the title [WIP] PR: Use of "hack" verboseTB [WIP] PR: Use selected syntax style for tracebacks and improve ANSI color codes support Jun 11, 2024
Copy link
Collaborator

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @jsbautista for the changes here. Left a comment regarding the change over the test. Let me know if you have questions!

qtconsole/tests/test_jupyter_widget.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @jsbautista and @dalthviz for your work on this!

Comment on lines 814 to 815
f"from IPython.core.ultratb import VerboseTB;"
"VerboseTB._tb_highlight_style = '{syntax_style}'",
Copy link
Collaborator

@ccordoba12 ccordoba12 Jun 13, 2024

Choose a reason for hiding this comment

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

A couple of comments about this change:

  • The _tb_highlight_style attribute was added in IPython 8.15 (see Add hack to change traceback syntax highlighting ipython/ipython#14138). So, it's necessary a check that executes the new code for that or more recent versions, and leaves the previous one for older versions.
  • It'd be a good idea to make _tb_highlight_style a public attribute of VerboseTB by submitting a PR to IPython. That would require an additional check here but it'd prevent they breaking us in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The _tb_highlight_style attribute was added in IPython 8.15 (see ipython/ipython#14138). So, it's necessary a check that executes the new code for that or more recent versions, and leaves the previous one for older versions.

That's quite a good point! Thinking about that, and the fact that this way of customizing things is based on a hack, I would say that we should implement the validation using some sort of fallback approach rather than checking the package version (maybe at some point that hack is removed in favor of an actual public method for customization). Maybe we could check if the attribute is available and depending on that use it or fallback to the previous code that uses the magic?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think something like this could work:

Suggested change
f"from IPython.core.ultratb import VerboseTB;"
"VerboseTB._tb_highlight_style = '{syntax_style}'",
f"""
from IPython.core.ultratb import VerboseTB
try:
VerboseTB._tb_highlight_style = '{syntax_style}'
except AttributeError:
get_ipython().run_line_magic('colors', '{colors}')
""",

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very good approach, thanks @dalthviz! However, given that _tb_highlight_style is a private attribute of VerboseTB, I think it'd be a good idea to promote it to a public one in IPython and let Matthias know that we're using it here.

Otherwise, this functionality could break easily without us being aware of it (given that now there's a fallback in case that attribute is no longer available).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Created ipython/ipython#14464 to check your suggestion @ccordoba12

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, thanks! I saw Matthias answer, so now please proceed to implement his suggestions there.

@dalthviz
Copy link
Collaborator

dalthviz commented Jun 18, 2024

Note: The latest failing tests on linux and mac are unrelated with the work here, seems like a message about the debugger and frozen modules (the one that mentions things like using PYDEVD_DISABLE_FILE_VALIDATION=1 and -Xfrozen_modules=off) is being caught when doing the tests related with communications between frontend and kernel/kernel and frontend

Edit: Multiple CI reruns helps things pass on linux but probably something to check. Maybe that is the reason why the related tests have a high number of retries (flacky marked with 10 tries) 🤔

qtconsole/mainwindow.py Outdated Show resolved Hide resolved
Co-authored-by: Daniel Althviz Moré <[email protected]>
@dalthviz dalthviz added this to the 5.6.0 milestone Jul 7, 2024
@dalthviz dalthviz changed the title [WIP] PR: Use selected syntax style for tracebacks and improve ANSI color codes support PR: Use selected syntax style for tracebacks and improve ANSI color codes support Aug 8, 2024
Copy link
Collaborator

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @jsbautista for your work here! Although ipython/ipython#14476 is still WIP I'm leaving this approved.

Just in case, do you have any further comments @ccordoba12? Should we wait until ipython/ipython#14476 gets merged to merge this or we could merge this regardless?

@dalthviz dalthviz changed the title PR: Use selected syntax style for tracebacks and improve ANSI color codes support Use selected syntax style for tracebacks and improve ANSI color codes support Aug 8, 2024
@ccordoba12
Copy link
Collaborator

Just in case, do you have any further comments @ccordoba12? Should we wait until ipython/ipython#14476 gets merged to merge this or we could merge this regardless?

Nop, I don't have more comments, so please merge it. If ipython/ipython#14476 goes in a different route, we can simply adapt the code here.

@dalthviz dalthviz merged commit f79480d into jupyter:main Aug 12, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants