rich icon indicating copy to clipboard operation
rich copied to clipboard

Introduce the uninstall method

Open noklam opened this issue 2 years ago • 7 comments
trafficstars

Type of changes

  • [ ] Bug fix
  • [x] 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 avatar May 25 '23 16:05 noklam

I'd accept a PR for an uninstall function. Originally posted by @willmcgugan in https://github.com/Textualize/rich/issues/2461#issuecomment-1211740277

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

noklam avatar Aug 15 '23 11:08 noklam

for the sanity of everyone everywhere please merge this

qrdlgit avatar Sep 10 '23 23:09 qrdlgit

@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.

davep avatar Sep 11 '23 04:09 davep

Related https://github.com/Textualize/rich/issues/2461

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

noklam avatar Sep 11 '23 08:09 noklam

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

davep avatar Sep 11 '23 08:09 davep

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.

willmcgugan avatar Sep 11 '23 17:09 willmcgugan

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 avatar Jul 02 '24 11:07 noklam