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.

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

Jump to bottom

Improve approx scalar repr method for better readability #12665

Merged
merged 17 commits into from
Aug 1, 2024
Merged

Improve approx scalar repr method for better readability #12665

merged 17 commits into from
Aug 1, 2024

Conversation

fazeelghafoor
Copy link
Contributor

@fazeelghafoor fazeelghafoor commented Jul 29, 2024 •
edited
Loading

Closes #6985

  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.
  • Add text like closes #XYZW to the PR description and/or commits (where XYZW is the issue number).
  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst.
  • Add yourself to AUTHORS in alphabetical order.

@fazeelghafoor fazeelghafoor changed the title resolve mypy error: Module has no attribute last_exc [attr-defined] Improve readability of approx method scalar __repre__ method for better readability Jul 29, 2024
@fazeelghafoor fazeelghafoor changed the title Improve readability of approx method scalar __repre__ method for better readability Improve readability of approx scalar repr method for better readability Jul 29, 2024
@fazeelghafoor fazeelghafoor marked this pull request as ready for review July 29, 2024 01:29
@fazeelghafoor
Copy link
Contributor Author

@Zac-HD please review and let me know if there are any changes required.

@fazeelghafoor fazeelghafoor changed the title Improve readability of approx scalar repr method for better readability Improve approx scalar repr method for better readability Jul 29, 2024
Pierre-Sassoulas
Pierre-Sassoulas reviewed Jul 29, 2024
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request !

Did you see this comment in the original issue : #6985 (comment) ?

@fazeelghafoor
Copy link
Contributor Author

@Pierre-Sassoulas thank you for the feedback. I had missed the comment so I have added the 'n' format specifier as well. Please let me know what else needs to changed

Pierre-Sassoulas
Pierre-Sassoulas requested changes Jul 29, 2024
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Some tests results to upgrade otherwise LGTM

@fazeelghafoor
Copy link
Contributor Author

added test cases as well

Pierre-Sassoulas
Pierre-Sassoulas requested changes Jul 30, 2024
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

The pipeline is still failing, some existing tests need to be upgraded too.

@fazeelghafoor
Copy link
Contributor Author

@Pierre-Sassoulas updated existing tests

Pierre-Sassoulas
Pierre-Sassoulas requested changes Jul 31, 2024
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Great, do you mind adding a changelog too, please ?

src/_pytest/runner.py Outdated
@@ -177,7 +177,7 @@ def pytest_runtest_call(item: Item) -> None:
sys.last_type = type(e)
sys.last_value = e
if sys.version_info >= (3, 12, 0):
sys.last_exc = e
sys.last_exc = e # type: ignore[attr-defined]
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this type ignores are unused in the CI (see pre-commit job).

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jul 31, 2024
nicoddemus
nicoddemus approved these changes Jul 31, 2024
View reviewed changes
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

LGTM, left some minor comments.

changelog/6985.improvement.rst Outdated Show resolved Hide resolved
changelog/6985.improvement.rst Outdated Show resolved Hide resolved
src/_pytest/python_api.py Outdated Show resolved Hide resolved
@fazeelghafoor
Copy link
Contributor Author

@nicoddemus thanks for the suggestions. can you let me know what could be causing the readthedocs check to fail. I don't understand the error

@The-Compiler
Copy link
Member

Looks like an issue unrelated to your PR: sphinx-contrib/sphinxcontrib-towncrier#92

@nicoddemus
Copy link
Member

can you let me know what could be causing the readthedocs check to fail. I don't understand the error

It might be some regression on towncrier/sphinx extension, I noticed there was a new release towncrier release 5 hrs ago:

https://pypi.org/project/towncrier/#history

Don't worry, seems unrelated to your changes.

The-Compiler
The-Compiler approved these changes Jul 31, 2024
View reviewed changes
Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Looking great!

@nicoddemus
Copy link
Member

Opened #12675 to fix the readthedocs/towncrier issue, we can update with main once that lands.

Pierre-Sassoulas
Pierre-Sassoulas reviewed Aug 1, 2024
View reviewed changes
changelog/6985.improvement.rst Outdated Show resolved Hide resolved
changelog/6985.improvement.rst Outdated Show resolved Hide resolved
fazeelghafoor and others added 2 commits August 1, 2024 22:07
@Pierre-Sassoulas
Copy link
Member

Great !

@nicoddemus nicoddemus merged commit 5a01583 into pytest-dev:main Aug 1, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

@The-Compiler The-Compiler The-Compiler approved these changes

@nicoddemus nicoddemus Awaiting requested review from nicoddemus

Assignees
No one assigned
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pytest.approx: Please improve repr readability
4 participants
@fazeelghafoor @The-Compiler @nicoddemus @Pierre-Sassoulas