-
Notifications
You must be signed in to change notification settings - Fork 88
This pull request fixes multiple Sphinx documentation warnings in the pyperf docs. #223
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi! 👋 All Sphinx documentation warnings have been resolved, and the docs build cleanly with -n (nitpicky mode). The changes are strictly documentation-related and do not affect runtime code. If everything looks good, I would appreciate a review or merge whenever convenient. Thank you! |
| .. function:: add_runs(filename: str, result) | ||
|
|
||
| Append a :class:`Benchmark` or :class:`BenchmarkSuite` to an existing | ||
| Append a :class:`pyperf.Benchmark` or :class:`pyperf.BenchmarkSuite` to an existing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you declared .. module:: pyperf, is it really needed to repeat pyperf. prefix here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again: please remove redundant pyperf. prefix.
|
|
||
| Format values including the unit. | ||
|
|
||
| .. method:: get_dates() -> (datetime.datetime, datetime.datetime) or None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the information about the return type. Write a sentence like:
"Return (datetime.datetime, datetime.datetime) or None."
|
|
||
| Get the total number of values. | ||
|
|
||
| .. method:: get_nwarmup() -> int or float |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the information about the return type. Would it work to use int | float?
Same remark for other methods below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't reply.
| -------------------------- | ||
|
|
||
| * Add *teardown* optional parameter to :class:`Runner.timeit` and ``--teardown`` | ||
| * Add *teardown* optional parameter to `Runner.timeit` and ``--teardown`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Add *teardown* optional parameter to `Runner.timeit` and ``--teardown`` | |
| * Add *teardown* optional parameter to ``Runner.timeit`` and ``--teardown`` |
| * Support PyPy | ||
| * metadata: add ``mem_max_rss`` and ``python_hash_seed`` | ||
| * Add :func:`perf.python_implementation` and :func:`perf.python_has_jit` | ||
| * Add `perf.python_implementation` and `perf.python_has_jit` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Add `perf.python_implementation` and `perf.python_has_jit` | |
| * Add ``perf.python_implementation`` and ``perf.python_has_jit`` |
Same remark below.
| } | ||
| nitpick_ignore = [ | ||
| ('py:class', 'int or float'), | ||
| ('py:class', '(datetime.datetime, datetime.datetime) or None'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it still needed since you removed them?
| show_name=True, | ||
| program_args=None, add_cmdline_args=None, | ||
| _argparser=None, warmups=1): | ||
| def is_worker(self) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't remove the method.
|
Hi @vstinner , thanks for the review! I have updated the PR to address all your feedback:
I ran the build locally ( Please let me know if there is anything else! |
I don't see your changes :-( |
Fixes #172
This PR resolves multiple Sphinx documentation warnings (reference target not found) that occurred when building the docs in nitpicky mode (-n).
🔧 Changes
Enabled Intersphinx: Updated doc/conf.py to include sphinx.ext.intersphinx, fixing broken links to standard library objects (e.g., datetime, float, sys.stdin).
Corrected API Namespace: Added .. module:: pyperf to doc/api.rst and updated class/method references to their fully qualified forms such as :class:pyperf.Benchmark and
:class:pyperf.Runner.Cleaned Up Method Signatures: Simplified or removed problematic type hints (e.g., -> int or float) in method signatures to prevent Sphinx parsing errors.
Stabilized Changelog References: Converted references to legacy or removed modules (e.g., perf, TextRunner) into literal code blocks to avoid link resolution failures.
Fixed Formatting Issues: Corrected indentation and list formatting errors in doc/changelog.rst that caused Sphinx warnings.
🧪 Verification
Documentation build verified locally using:
sphinx-build -n -b html doc/ _build/html
Result: Documentation now builds with zero warnings (previously ~46).