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

[highlighter] Add complex numbers pattern to our highlighter's number matching #2219

Merged
merged 5 commits into from
Apr 26, 2022

Conversation

olivierphi
Copy link

@olivierphi olivierphi commented Apr 25, 2022

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

Description

closes #2214

The fix in action:

In IPython:

Screenshot from 2022-04-25 12-24-00

In a Jupyter notebook:

Screenshot from 2022-04-25 12-28-04

In plain Python

Screenshot from 2022-04-25 12-29-03

@olivierphi olivierphi force-pushed the bugfix-inconsistent-coloring-of-complex-numbers branch from 0d0e621 to dda6ce5 Compare April 25, 2022 11:33
@olivierphi olivierphi marked this pull request as ready for review April 25, 2022 11:37
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

I don't think this will work with negative imaginary numbers, e.g. "1-3j"

@olivierphi olivierphi force-pushed the bugfix-inconsistent-coloring-of-complex-numbers branch from dda6ce5 to f67f426 Compare April 25, 2022 13:51
@olivierphi
Copy link
Author

I don't think this will work with negative imaginary numbers, e.g. "1-3j"

Meh, silly me 😓 - as I've never used these numbers myself I naively assumed that they were always an imaginary part added to the real one, but of course it can also be a subtraction! Fixed in f67f426
Good spot, thanks for the review! 🙂

Also... While I was there I checked Wikipedia once again, and I found this:

Moreover, when the imaginary part is negative, that is, b = −|b| < 0, it is common to write a − |b|i instead of a + (−|b|)i; for example, for b = −4, 3 − 4i can be written instead of 3 + (−4)i.

But it seems that in Python this 3 + (−4)j is not a valid syntax, so no need to handle that I suppose...

@willmcgugan
Copy link
Collaborator

No worries. We only really need to worry about what the repr for complex numbers can produce.

I'm afraid it does get more complex. If the imaginary part has an exponent, the regex fails.

e.g. (-123123-2.1312342342423422e+25j)

I suspect that will occur almost never, but maybe we should try to support it.

I'm actually thinking that maybe the best approach would be to add a new regex for a new number type, which includes the parenthesis. So it matches (<NUMBER>(+/-)<NUMBER>j). What do you think?

@olivierphi
Copy link
Author

I'm actually thinking that maybe the best approach would be to add a new regex for a new number type, which includes the parenthesis. So it matches (<NUMBER>(+/-)<NUMBER>j). What do you think?

I'm trying to implement this, but without any success so far... I have a regex that works in isolation in regex101 with several test cases of complex numbers, but once I mix it with the other ones we already have in the highlighter it breaks quite a lot of other test cases 😬
But I'll keep trying! 🤞 🙂

@olivierphi
Copy link
Author

I made some progress, but still not there for the moment... 😅
Now my issue is that the "number" and "brace" patterns seem to take always precedence over the new "complex_number" one - which seem to make sense, at least for the brace, since (<NUMBER>(+/-)<NUMBER>j) does contain braces. Not sure what the best way to solve this could be? 🤔

@olivierphi
Copy link
Author

@willmcgugan I think I got there! It seems that I cannot prevent the "braces" to take precedence (even if I move the new complex_number matcher at the same level than it, outside of the _combine_regex() call), but apart from that it seems to work.

Here with a different visual style than "plain" numbers, just to illustrate that the matching is well and truly different:
Screenshot from 2022-04-25 16-44-08

What do you think? Good enough, or should we try harder to include the braces into this representation? 🙂

…n one, rather than being part of the `number` one
@olivierphi olivierphi force-pushed the bugfix-inconsistent-coloring-of-complex-numbers branch from 22be33e to 5e67e22 Compare April 25, 2022 15:57
@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2022

Codecov Report

Merging #2219 (69003a7) into master (46b1c95) will decrease coverage by 0.17%.
The diff coverage is 82.07%.

@@            Coverage Diff             @@
##           master    #2219      +/-   ##
==========================================
- Coverage   99.07%   98.89%   -0.18%     
==========================================
  Files          72       73       +1     
  Lines        7555     7625      +70     
==========================================
+ Hits         7485     7541      +56     
- Misses         70       84      +14     
Flag Coverage Δ
unittests 98.89% <82.07%> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rich/default_styles.py 100.00% <ø> (ø)
rich/highlighter.py 100.00% <ø> (ø)
rich/layout.py 100.00% <ø> (ø)
rich/markup.py 100.00% <ø> (ø)
rich/__init__.py 93.54% <60.00%> (-6.46%) ⬇️
rich/progress.py 92.73% <79.56%> (-0.79%) ⬇️
rich/progress_bar.py 99.00% <85.71%> (-1.00%) ⬇️
rich/console.py 98.89% <86.36%> (-0.33%) ⬇️
rich/segment.py 98.71% <87.50%> (-0.31%) ⬇️
rich/_export_format.py 100.00% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65227ba...69003a7. Read the comment docs.

@olivierphi
Copy link
Author

olivierphi commented Apr 25, 2022

Also, I just discovered that 3.4j is a valid imaginary number, and in that case (only an imaginary part) there are no braces in its representation!
Screenshot from 2022-04-25 17-03-42

rich/default_styles.py Outdated Show resolved Hide resolved
@willmcgugan
Copy link
Collaborator

Nice work. If the braces aren't technically part of the number then it makes sense to leave them out of the regex.

Not sure about the styling. @flaport do you have any particular preference re styling? Would you expect it to look different from a non-complex number?

rich/highlighter.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

Would you mind adding an entry in CHANGELOG.md with a link to the issue

@olivierphi
Copy link
Author

olivierphi commented Apr 26, 2022

Would you mind adding an entry in CHANGELOG.md with a link to the issue

Sure! Done in 9ae04d2 🙂

@willmcgugan willmcgugan merged commit a7e8a6b into master Apr 26, 2022
@willmcgugan willmcgugan deleted the bugfix-inconsistent-coloring-of-complex-numbers branch April 26, 2022 10:35
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.

[BUG] inconsistent coloring of complex numbers
3 participants