Add Sphinx-compatible docstrings and inline comments for main.py (lines 245-490) #13091

mohammadaminjohari · 2024-12-29T22:28:48Z

This pull request addresses issue #12979 by enhancing the documentation and readability of the file pytest/src/_pytest/main.py in the range of lines 245 to 490.
Changes Made:

Docstrings Added:
    All functions and classes in the specified range now include Sphinx-compatible docstrings with:
        A brief overview of the purpose of each function or class.
        Detailed descriptions of parameters (names, types, and expected values).
        Return values and their types.
        Possible exceptions raised by the functions.


    Added meaningful inline comments to clarify complex logic and improve code readability.

Functions and Classes Updated:

validate_basetemp: Validates the base temporary directory path.
wrap_session: Executes the pytest session in a controlled wrapper.
pytest_cmdline_main: The main entry point for pytest command-line execution.
_main: The default protocol for session initialization, test running, and reporting.
pytest_collection: Handles the collection phase of pytest sessions.
pytest_runtestloop: Manages the test run loop for pytest sessions.



All changes were verified locally by running the test suite using tox. No errors or warnings were encountered.



Please review the changes and let me know if further modifications are needed.

Best regards,
Mohammadamin Johari

for more information, see https://pre-commit.ci

dongfangtianyu

Thanks @mohammadaminjohari for PR, just 2 suggestions for reference:

Keep the style of the docstring consistent with the existing pytest code, for example,
- content: https://docs.pytest.org/en/stable/_modules/_pytest/hookspec.html#pytest_addoption,
- html: https://docs.pytest.org/en/stable/reference/reference.html#pytest.hookspec.pytest_addoption
Do not modify the inline comments for now, it is not necessary at first glance

dongfangtianyu · 2024-12-31T05:31:52Z

src/_pytest/main.py

@@ -248,24 +248,37 @@ def pytest_addoption(parser: Parser) -> None:


 def validate_basetemp(path: str) -> str:
-    # GH 7119
+    """
+    Validate the base temporary directory path.


GH 7119 refers to #7119 clarifying the origin of the code, suggest keeping it in the docstrings, for example:

Validate the base temporary directory path (see also :issue:`7119`).

dongfangtianyu · 2024-12-31T05:35:32Z

src/_pytest/main.py

@@ -275,17 +288,26 @@ def is_ancestor(base: Path, query: Path) -> bool:
 def wrap_session(
    config: Config, doit: Callable[[Config, Session], int | ExitCode | None]
 ) -> int | ExitCode:
-    """Skeleton command line program."""
+    """
+    Execute the pytest session within a controlled wrapper.


The purpose is to execute doit, not to execute session，
Consider modifying to:

Execute a function within a pytest session.

dongfangtianyu · 2024-12-31T05:37:10Z

src/_pytest/main.py

            initstate = 2
-            session.exitstatus = doit(config, session) or 0
+            session.exitstatus = doit(config, session) or 0  # Execute main logic


now，execute doit, not execute main. (These line comments may be unnecessary because the method names are already intuitive enough)

Pierre-Sassoulas

Thanks you for opening the PR. I started a review but @dongfangtianyu did something more detailled, i'll just leave what I had. About inline comment in particular : Commentting what the code does is duplicating informations, inline comment should be about why the code is done this way not what it does.

Pierre-Sassoulas · 2024-12-30T06:35:33Z

src/_pytest/main.py

+        argparse.ArgumentTypeError: If the path is invalid, such as being empty, the
+        current working directory, or a parent directory of the current working directory.
+    """
+    # Error message for invalid paths


Suggested change

# Error message for invalid paths

Pierre-Sassoulas · 2024-12-30T06:38:00Z

src/_pytest/main.py

        excinfo = None  # type: ignore
-        os.chdir(session.startpath)
+        os.chdir(session.startpath)  # Return to the initial directory


Suggested change

os.chdir(session.startpath) # Return to the initial directory

os.chdir(session.startpath)

Pierre-Sassoulas · 2024-12-30T06:38:16Z

src/_pytest/main.py

            initstate = 2
-            session.exitstatus = doit(config, session) or 0
+            session.exitstatus = doit(config, session) or 0  # Execute main logic


Suggested change

session.exitstatus = doit(config, session) or 0 # Execute main logic

session.exitstatus = doit(config, session) or 0

Pierre-Sassoulas · 2024-12-30T06:38:36Z

src/_pytest/main.py

            initstate = 1
-            config.hook.pytest_sessionstart(session=session)
+            config.hook.pytest_sessionstart(session=session)  # Notify start of session


Suggested change

config.hook.pytest_sessionstart(session=session) # Notify start of session

config.hook.pytest_sessionstart(session=session)

Pierre-Sassoulas · 2024-12-30T06:38:52Z

src/_pytest/main.py

    session = Session.from_config(config)
    session.exitstatus = ExitCode.OK
    initstate = 0
    try:
        try:
-            config._do_configure()
+            config._do_configure()  # Initialize pytest configuration


Suggested change

config._do_configure() # Initialize pytest configuration

config._do_configure()

Pierre-Sassoulas · 2024-12-30T11:51:34Z

src/_pytest/main.py

    msg = "basetemp must not be empty, the current working directory or any parent directory of it"

-    # empty path
+    # Check for an empty path


Suggested change

# Check for an empty path

dongfangtianyu · 2025-01-02T05:19:21Z

Hi @mohammadaminjohari ,
I received a reminder of the review request, but did not see any new changes. If available, you can continue to commit and push on the this branch.

mohammadaminjohari and others added 3 commits December 30, 2024 01:53

Update main.py

504ee2f

[pre-commit.ci] auto fixes from pre-commit.com hooks

8e12914

for more information, see https://pre-commit.ci

Create 12979.doc.rst

7126d14

psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Dec 29, 2024

mohammadaminjohari mentioned this pull request Dec 30, 2024

Add Missing Docstrings and inline comments to pytest/src/_pytest/main.py (Lines 245-490) #12979

Open

dongfangtianyu suggested changes Dec 31, 2024

View reviewed changes

Pierre-Sassoulas reviewed Dec 31, 2024

View reviewed changes

mohammadaminjohari requested a review from dongfangtianyu January 1, 2025 06:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sphinx-compatible docstrings and inline comments for main.py (lines 245-490) #13091

Add Sphinx-compatible docstrings and inline comments for main.py (lines 245-490) #13091

mohammadaminjohari commented Dec 29, 2024

dongfangtianyu left a comment

dongfangtianyu Dec 31, 2024 •

edited

Loading

dongfangtianyu Dec 31, 2024

dongfangtianyu Dec 31, 2024

Pierre-Sassoulas left a comment

Pierre-Sassoulas Dec 30, 2024

Pierre-Sassoulas Dec 30, 2024

Pierre-Sassoulas Dec 30, 2024

Pierre-Sassoulas Dec 30, 2024

Pierre-Sassoulas Dec 30, 2024

Pierre-Sassoulas Dec 30, 2024

dongfangtianyu commented Jan 2, 2025

	os.chdir(session.startpath) # Return to the initial directory
	os.chdir(session.startpath)

	session.exitstatus = doit(config, session) or 0 # Execute main logic
	session.exitstatus = doit(config, session) or 0

	config.hook.pytest_sessionstart(session=session) # Notify start of session
	config.hook.pytest_sessionstart(session=session)

	config._do_configure() # Initialize pytest configuration
	config._do_configure()

Add Sphinx-compatible docstrings and inline comments for main.py (lines 245-490) #13091

Are you sure you want to change the base?

Add Sphinx-compatible docstrings and inline comments for main.py (lines 245-490) #13091

Conversation

mohammadaminjohari commented Dec 29, 2024

dongfangtianyu left a comment

Choose a reason for hiding this comment

dongfangtianyu Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongfangtianyu commented Jan 2, 2025

dongfangtianyu Dec 31, 2024 •

edited

Loading