Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

DOC: add section about how to write good error messages#25292

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
tacaswell wants to merge1 commit intomatplotlib:main
base:main
Choose a base branch
Loading
fromtacaswell:doc/verbose_errors

Conversation

@tacaswell
Copy link
Member

PR Summary

After leaving a comment to make error messages verbose I felt inspired to write it up for the docs...

Adding generated API docs for the private API might be a bit controversial, but I think on net it is the right thing to do so that we if we do remove them we can be sure that the docs do not still advocate for their use and saying "there are somecheck_* functions" felt too vague).

melissawm reacted with hooray emoji
@tacaswelltacaswell added this to thev3.8.0 milestoneFeb 22, 2023
@tacaswelltacaswell mentioned this pull requestFeb 22, 2023
6 tasks
While this does tell the user what variable and value Matplotlib rejected, it still
does not tell them why.

An example of a **best** error message ::
Copy link
Member

Choose a reason for hiding this comment

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

Throwing your advice back to you, what about starting with the recommended best practice so that it won't get lost in skimming/fatigue? Then if it's still necessary, discuss unpack why the bad is bad - but honestly I'm not sure it's necessary given I think the paragraph under here does a good job of explaining why folks should do it this way.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with this - the first draft is pretty verbose....

Comment on lines 590 to 591
Which tells the user what input in broken, what was passed in, and the reason
why the value was invalid. All of this together gives the user the best chance
Copy link
Member

Choose a reason for hiding this comment

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

This is a personal preference, but I like this sort of info broken out into a list:

We recommend writing an error message that states:

  1. which parameter is raising the error
  2. what value was passed
  3. why the value is incorrect

Comment on lines 570 to 572
In cases like this it may be possible to get into an interactive debugger and
from reading the stack trace sort out right local variable to check is to start
to sort out what went wrong. However, without access to the running process the user
Copy link
Member

Choose a reason for hiding this comment

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

After theand, this sentence kind of gets a bit muddled.

Internal helpers
~~~~~~~~~~~~~~~~

Matplotlib has number of helper functions in the ``matplotlib._api`` module
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Matplotlib has number of helper functions in the ``matplotlib._api`` module
Matplotlib hasanumber of helper functions in the ``matplotlib._api`` module

~~~~~~~~~~~~~~~~

Matplotlib has number of helper functions in the ``matplotlib._api`` module
named as ``check_*`` which provide nice error messages for very common checks
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
named as ``check_*`` which provide nice error messages forverycommon checks
named as ``check_*`` which provide nice error messages for common checks

very seems superfluous?


This message tells the user what (local) variable Matplotlib checked was and
what value was passed in which can help users rapidly find bugs in their own
code. For example if they expected to have passed in an integer but the error
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
code. For example if they expected to have passed in an integer but the error
code. For example, if they expected to have passed in an integer but the error

This message tells the user what (local) variable Matplotlib checked was and
what value was passed in which can help users rapidly find bugs in their own
code. For example if they expected to have passed in an integer but the error
messages reports the string ``'aardvark'``, they may know where in their code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
messages reports the string ``'aardvark'``, they may know where in their code
message reports the string ``'aardvark'``, they may know where in their code

@oscargus
Copy link
Member

I'm missing info about dropping punctuation characters and if an error message should be in Sentence case or nor.

@anntzer
Copy link
Contributor

Highly nitpicky, but my personal(?) view is to do what cpython does:

>>> object()()Traceback (most recent call last):  File "<stdin>", line 1, in <module>TypeError: 'object' object is not callable>>> object().barTraceback (most recent call last):  File "<stdin>", line 1, in <module>AttributeError: 'object' object has no attribute 'bar'>>> getattr(object(), 42)Traceback (most recent call last):  File "<stdin>", line 1, in <module>TypeError: attribute name must be string, not 'int'

i.e. no leading capital and no trailing punctuation.
However, if the error message spans multiple sentences, then all sentences should be capitalized and punctuated correctly. I try to avoid such cases, e.g. I would reword the error message of check_shape (currently'arg' must be 2D with shape (M, 2). Your input has shape (3, 3).) to'arg' must be 2D with shape (M, 2); your input has shape (3, 3)

@anntzer
Copy link
Contributor

anntzer commentedFeb 23, 2023•
edited
Loading

I agree with others that this seems pretty verbose (though a good idea to document); a first try at editing (while keeping most of your wording) gives me

Error messages should err on the side of being verbose, descriptive, andspecific about what went wrong.  They should be informative enough so that atypical user (not an expert of our API) can understand, debug, and fix theirproblem based solely on the message, without the need to consult the onlinedocumentation.For example, input validation errors should include, where possible,information about which input is invalid, what value was passed in, and why thevalue is invalid, e.g. ::    raise ValueError(f"{value=!r} is invalid, value must be a positive integer")This message helps the user to quickly diagnose and fix their problem.Remember that many other libraries are implemented on top of Matplotlib, andtherefore ``value`` may not even have been directly passed by the user, butrather generated in some intermediate layer.  In such cases, this kind ofhighly explicit messages can be particularly useful, compared to shortermessages such as ::    raise ValueError("invalid value")
story645 reacted with thumbs up emoji

@tacaswell
Copy link
MemberAuthor

I took@anntzer 's wording whole sale and applied the other changes.

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>Co-authored-by: Antony Lee <anntzer.lee@gmail.com>Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
@story645
Copy link
Member

story645 commentedOct 9, 2025•
edited
Loading

Can you leave licensing as the last entry since it's not really like the others?

information about which input is invalid, what value was passed in, and why the
value is invalid, e.g. ::

raise ValueError(f"{value=!r} is invalid, value must be a positive integer")
Copy link
Member

@timhoffmtimhoffmOct 10, 2025•
edited
Loading

Choose a reason for hiding this comment

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

I suggest to include the parameter name. It's not always needed, but good for educational purposes. For example

raiseValueError("rotation must be 'vertical', 'horizontal' or "
f"a number, not{s}")

Edit, oh maybe you meanvalue as the parameter name? In that case I would use a more targeted name for the example.

We may also consider adopting a policy to enquote parameter names in single ticks. While there's no universal convention, this is the closest I have found for a pattern, see e.g.

Comment on lines +347 to +349
messages such as ::

raise ValueError("invalid value")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe inline to de-emphasize?

Suggested change
messages such as ::
raise ValueError("invalid value")
messages such as ``raise ValueError("invalid value")``.

When skimming the section, you primarily notice the heading and the two code blocks, giving the impression that both are relevant. There's no indication that one is a negative example.

image

Comment on lines +364 to +375
..currentmodule::matplotlib


..autosummary::
:toctree: _as_gen
:template: autosummary.rst
:nosignatures:

_api.check_isinstance
_api.check_in_list
_api.check_shape
_api.check_getitem
Copy link
Member

Choose a reason for hiding this comment

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

This generates API documentation resulting in duplication

The best would be if one could tell autosummary not to create new API doecumentation and just link to the existing ones, but I think that is not possible (correct me if I'm wrong).

As a workaround, I'd create a manual table or bullet list (with or without copied/hard-coded description).

An alternative with a little more work would be to structurehttps://matplotlib.org/stable/api/_api_api.html into topic sections and and link to the section on validation/error handling.

Internal helpers
~~~~~~~~~~~~~~~~

Matplotlib has a number of helper functions in the ``matplotlib._api`` module
Copy link
Member

Choose a reason for hiding this comment

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

link

Suggested change
Matplotlib has a number of helper functions in the ``matplotlib._api`` module
Matplotlib has a number of helper functions in the `matplotlib._api` module

Comment on lines +359 to +361
consistent across the library. Use them in your own code at your own risk, we
consider them private API and reserve the right change them with no notice on
any release.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
consistent across the library. Use them in your own code at your own risk, we
consider them private API and reserve the right change them with no notice on
any release.
consistent across the library.

I would leave this out here. It's in an underscored module and the module docstring also explicitly states this.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@QuLogicQuLogicQuLogic left review comments

@story645story645story645 left review comments

@jklymakjklymakjklymak left review comments

@ksundenksundenksunden left review comments

@timhoffmtimhoffmtimhoffm left review comments

@oscargusoscargusoscargus left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Projects

None yet

Milestone

v3.8-doc

Development

Successfully merging this pull request may close these issues.

8 participants

@tacaswell@oscargus@anntzer@story645@QuLogic@jklymak@ksunden@timhoffm
[8]ページ先頭
©2009-2025 Movatter.jp