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

Introduce the uninstall method #2972

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

noklam
Copy link
Contributor

@noklam noklam commented May 25, 2023

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

Description

Related #2461, #1947

This is a very rough draft but it's a working solution. List of questions that I have

  • Is manual testing enough? I am not sure how can I write an automated test for this and need some help
  • Previously when running install for IPython, it returns a sys.excepthook which isn't useful, because it's just a pointer to the object and the old hooks that we want to preserve are gone.
  • It's also not very consistent now with this change as for Python path, it returns a hook but for IPython path it returns a list of method which need to be recover when uninstall
  • Should a global variable or a stateful class be used? This will make it easier to use rich.traceback.uninstall(), but it will also be a breaking change so I haven't implement in this way yet, would love to get some feedback.

Manual Test

Python

image

IPython

image

Please describe your changes here. If this fixes a bug, please link to the issue, if possible.
I have tested this on Github workspace, you can use this script to verify the behavior.

python
import rich.traceback

hooks = rich.traceback.install()
rich.traceback.uninstall(hooks)
error # trigger error
ipython

import rich.traceback

hooks = rich.traceback.install()
rich.traceback.uninstall(hooks)
error # trigger error

@noklam
Copy link
Contributor Author

noklam commented Aug 15, 2023

I'd accept a PR for an uninstall function.
Originally posted by @willmcgugan in #2461 (comment)

Pinging for review/comments, do you think this is still valuable?

@qrdlgit
Copy link

qrdlgit commented Sep 10, 2023

for the sanity of everyone everywhere please merge this

@davep
Copy link
Contributor

davep commented Sep 11, 2023

@qrdlgit Could I ask you to read over the code of conduct and reflect on your approach and language: https://github.com/Textualize/rich/blob/master/CODE_OF_CONDUCT.md

I'm sure whatever issues you've run into, due to the decisions made by the authors of the libraries you've chosen to install, are frustrating; but using Rich issues to vent at the authors and maintainers of Rich isn't going to solve anything.

@noklam
Copy link
Contributor Author

noklam commented Sep 11, 2023

Related #2461

Hey @davep , any chance that I can get some feedback on the PR?

@davep
Copy link
Contributor

davep commented Sep 11, 2023

@noklam Not something I'm in a position to help with I'm afraid.

@willmcgugan
Copy link
Collaborator

You are hooking into the users environment and changing it so that you live beyond the lifetime of the installed packages / processes the user wants to use.

This is nonsense.

Your belligerent tone makes me disinclined to unburden you of your ignorance. If you want help, post a new issue (do not reply to this issue). But bear in mind you are talking to real people who have worked very hard to bring you some amazing software for free.

@Textualize Textualize deleted a comment from qrdlgit Sep 11, 2023
@Textualize Textualize deleted a comment from qrdlgit Sep 11, 2023
@Textualize Textualize deleted a comment from qrdlgit Sep 11, 2023
ipy_excepthook_closure(ip)
return sys.excepthook
return old_hooks
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method now returns two quite different return types, if I read this right.

Feels like a code smell. How about we return a callable which uninstalls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey! Thanks for having a look at this. It has been a while so I have forgotten some details. Let me have a look today and come back to this.

except Exception:
# otherwise use default system hook
old_excepthook = sys.excepthook
sys.excepthook = excepthook
return old_excepthook


def uninstall(hooks):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better for this method not to require a parameter. If we return an uninstall callable, perhaps it can be stored as a global and called here.

I'll also need a docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment, I have made some changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to do something similar with rich.pretty.install as well

@noklam
Copy link
Contributor Author

noklam commented Jul 2, 2024

Run source $VENV
black --check .
would reformat rich/tree.py

Oh no! 💥 💔 💥
1 file would be reformatted, 189 files would be left unchanged.
make: *** [Makefile:6: format-check] Error 1
Error: Process completed with exit code 2.

I see a linting error but doesn't seem to be related to my change.

@noklam noklam requested a review from willmcgugan July 2, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants