FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Changed fstring to make error clearer · Pull Request #21968 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Changed fstring to make error clearer #21968

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.

Sign up for GitHub

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

Closed
ghost wants to merge 6 commits into from
Conversation 17 Commits 6 Checks 0 Files changed
Closed

Changed fstring to make error clearer #21968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d9518d7
Changed fstring to make error clearer
Dec 15, 2021
23b8f4d
Merge branch 'main' of https://github.com/matplotlib/matplotlib into …
Dec 16, 2021
a527add
Added test for color value error
Dec 16, 2021
eee434e
Update test_axes.py
Dec 16, 2021
7beb751
Update test_axes.py
Dec 16, 2021
9571213
Update test_axes.py
Dec 17, 2021
File filter

Filter by extension

Filter by extension .py  (2) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
2 changes: 1 addition & 1 deletion lib/matplotlib/axes/_axes.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -4236,7 +4236,7 @@ def invalid_shape_exception(csize, xsize):
# severe failure => one may appreciate a verbose feedback.
raise ValueError(
f"'c' argument must be a color, a sequence of colors, "
f"or a sequence of numbers, not {c}") from err
f"or a sequence of numbers, not {c!r}") from err
else:
if len(colors) not in (0, 1, xsize):
# NB: remember that a single color is also acceptable.
Expand Down
2 changes: 2 additions & 0 deletions lib/matplotlib/tests/test_axes.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -2212,6 +2212,8 @@ def test_scatter_color(self):
plt.scatter([1, 2], [1, 2], color=[0.1, 0.2])
with pytest.raises(ValueError):
plt.scatter([1, 2, 3], [1, 2, 3], color=[1, 2, 3])
with pytest.raises(ValueError, match=f"{c!r}"):

yuanx749 Dec 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I have encountered this issue, and thanks for the PR! Would this simple test below work? @sveneschlbeck @tacaswell

Suggested change
with pytest.raises(ValueError, match=f"{c!r}"):
with pytest.raises(ValueError, match=r"\\n|\\t|\\r"):
plt.scatter([1, 2], [1, 2], c='blue\n')

ghost Dec 21, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@yuanx749 Thanks for the input...
@tacaswell Is that still the same logic? I think this is not exactly the same/equivalent message, right?

tacaswell Dec 23, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Picking the conditions on the test is a bit of an art.

Too specific and you can end up with a really brittle test suite that breaks on tiny changes that no user will ever care about. A test suite that is always broken or you are always having to change with the code is not really doing its job and causing you a whole lot of work!

Too general it will always pass, even if you make changes that significantly break users. This is also not great as again you test suite is not doing its job and still causing you a whole bunch of work.

As written the tests are failing because https://github.com/matplotlib/matplotlib/runs/4566462001?check_suite_focus=true#step:12:142

________________________ TestScatter.test_scatter_color ________________________
143
[gw1] linux -- Python 3.10.1 /opt/hostedtoolcache/Python/3.10.1/x64/bin/python

self = <matplotlib.tests.test_axes.TestScatter object at 0x7f345c9acbb0>
    def test_scatter_color(self):
        # Try to catch cases where 'c' kwarg should have been used.
        with pytest.raises(ValueError):
            plt.scatter([1, 2], [1, 2], color=[0.1, 0.2])
        with pytest.raises(ValueError):
            plt.scatter([1, 2, 3], [1, 2, 3], color=[1, 2, 3])
>       with pytest.raises(ValueError, match=f"{c!r}"):
E       NameError: name 'c' is not defined
lib/matplotlib/tests/test_axes.py:2215: NameError

in the namespace where the fstring is being evaluated we do not have c defined. You could change the test so the you do

c = 'blue\n'
with pytest.raises(ValueError, match=f"{c!r}"):
    plt.scatter([1, 2], [1, 2], c=c)

which tests that the literal string 'blue\\n' shows up. With @yuanx749 's suggestion you do not have to change anything else and it checks that at least one of new line, tab, or carriage return is literally in the message.

Between these two I would lean slightly towards searching for 'blue\n' (assume the escaping is right), in case we in the future add a note like "be careful you do not have a trailing \t or \n!" into the error message. In that case we would be able to stop putting the repr in and still have the test pass. However, I would be happy to merge this in either case so long as the tests pass now, the new test would fail without your other changes, and you can make a case as to why you wrote the test the way you did if asked.
plt.scatter([1, 2], [1, 2], color='\n')

def test_scatter_unfilled(self):
coll = plt.scatter([0, 1, 2], [1, 3, 2], c=['0.1', '0.3', '0.5'],
Expand Down
Back | FazBrowse Home | New Git URL