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

bpo-41510: Updated documentation to add header kw-argument of pdb.set_trace() #21793

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ShubhamKJha
Copy link

@ShubhamKJha ShubhamKJha commented Aug 9, 2020

Copy link
Contributor

@m-aciek m-aciek left a comment

In my (humble) opinion we can omit that extra information about new argument of set_trace in this contexts. It would be great if someone else (maybe some core commiter) expressed his (her) opinion on that.

:func:`pdb.set_trace()` expecting an optional keyword argument `header`. In
this case, it is purely a convenience function so you don't have to explicitly
import :mod:`pdb` or type as much code to enter the debugger. However,
:func:`sys.breakpointhook` can be set to some other function and
:func:`breakpoint` will automatically call that, allowing you to drop into
the debugger of choice.
Copy link
Contributor

@m-aciek m-aciek Aug 9, 2020

Choose a reason for hiding this comment

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

I'm not an expert, but I wonder if we can leave those details behind.

Suggested change
:func:`pdb.set_trace()` expecting an optional keyword argument `header`. In
this case, it is purely a convenience function so you don't have to explicitly
import :mod:`pdb` or type as much code to enter the debugger. However,
:func:`sys.breakpointhook` can be set to some other function and
:func:`breakpoint` will automatically call that, allowing you to drop into
the debugger of choice.
:func:`pdb.set_trace()`, however it can be set to some other function and
:func:`breakpoint` will automatically call that, allowing you to drop into
the debugger of choice.

Copy link
Author

@ShubhamKJha ShubhamKJha Aug 14, 2020

Choose a reason for hiding this comment

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

I understand, but if we are providing information about the standard hook then we must provide what behavior it would show (i.e, arguments' info)

@@ -189,10 +189,10 @@ always available.
function so that you can choose which debugger gets used.

The signature of this function is dependent on what it calls. For example,
Copy link
Contributor

@m-aciek m-aciek Aug 9, 2020

Choose a reason for hiding this comment

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

Here also I would suggest dropping that extra information.

Suggested change
The signature of this function is dependent on what it calls. For example,
The signature of this function is dependent on what it calls. The built-in
``breakpoint()`` function passes its ``*args`` and ``**kws`` straight through.
Whatever ``breakpointhooks()`` returns is returned from ``breakpoint()``.

:func:`pdb.set_trace()` expecting no arguments. In this case, it is
purely a convenience function so you don't have to explicitly import
:mod:`pdb` or type as much code to enter the debugger. However,
:func:`pdb.set_trace()` expecting an optional keyword argument `header`. In
Copy link
Member

@nanjekyejoannah nanjekyejoannah Aug 13, 2020

Choose a reason for hiding this comment

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

My eyes may be wrong but there might be an extra space before "In".

Copy link
Author

@ShubhamKJha ShubhamKJha Aug 14, 2020

Choose a reason for hiding this comment

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

In .rst files I guess there must be 2 spaces after .

Copy link
Member

@nanjekyejoannah nanjekyejoannah Aug 14, 2020

Choose a reason for hiding this comment

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

ohh I see. Thanks.

Copy link
Contributor

@m-aciek m-aciek Aug 15, 2020

Choose a reason for hiding this comment

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

https://devguide.python.org/documenting/#use-of-whitespace

A sentence-ending period may be followed by one or two spaces; while reST ignores the second space, it is customarily put in by some users, for example to aid Emacs’ auto-fill mode.

@ShubhamKJha
Copy link
Author

ShubhamKJha commented Aug 14, 2020

Hi @nanjekyejoannah this can be merged, I guess.

Copy link
Member

@nanjekyejoannah nanjekyejoannah left a comment

I will cc @ronaldoussoren for an opinion.

Copy link
Contributor

@ronaldoussoren ronaldoussoren left a comment

I'm leaving my feedback as a comment because I don't have really clear feedback.

I wonder if changing the text is necessary, other than maybe a slight rewording because the current text says the default hook won't accept any arguments which is untrue. I wouldn't document the keyword argument of pdb.set_trace here, IMHO that just makes the documentation harder to understand.

Note that the breakpoint function is intended to make it easy to drop in a debugger. I'd expect that most of not all calls to the breakpoint function will have no arguments.

:func:`pdb.set_trace()` expecting no arguments. In this case, it is
purely a convenience function so you don't have to explicitly import
:mod:`pdb` or type as much code to enter the debugger. However,
:func:`pdb.set_trace()` expecting an optional keyword argument `header`. In
Copy link
Contributor

@ronaldoussoren ronaldoussoren Aug 15, 2020

Choose a reason for hiding this comment

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

I'm not a native english speaker, but "expecting an optional ..." doesn't feel right.

I'm not sure if anything needs to change here, expect maybe to rephrase the current text to indicate that the default hook can be called without arguments. Users can look up the documentation of pdb.set_trace for advanced usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants