hoh / reloadr

Hot code reloading tool for Python
GNU Lesser General Public License v3.0
529 stars 12 forks source link

Python 3.9 incompatibility "class must be set to a class, not 'ClassReloadr' object" #13

Open zhangyq73 opened 3 years ago

zhangyq73 commented 3 years ago

I run 01_manual_reload.py in the examples directory,error is as follows: TypeError: class must be set to a class, not 'ClassReloadr' object python-BaseException. I am using python3.9 on the win10 platform,What is the cause of this error?

hoh commented 3 years ago

Looks like Python 3.9 changes the way it handles classes internally.

I can reproduce the issue using Python 3.9 on Linux using Docker image python:3.9-buster. The example works fine using Python 3.8 via python:3.9-buster.

Traceback:

$ python 01_manual_reload.py 
Car on 1001 3001
Traceback (most recent call last):
  File "reloadr/examples/01_manual_reload.py", line 28, in <module>
    Car._reload()
  File "/usr/local/lib/python3.9/site-packages/reloadr.py", line 136, in _reload
    instance.__class__ = self._target
TypeError: __class__ must be set to a class, not 'ClassReloadr' object
hoh commented 3 years ago

The issue comes from https://github.com/hoh/reloadr/blob/master/reloadr.py#L31 : in Python 3.9, inspect.getsource(class_object) returns the decorators as well.

Below is a temporary patch. The issue of decorators should be handled in a better way.

-    if kind == 'def':
-        # `inspect.getsource` will not return the decorators in the source of classes,
-        # but will return them in the source of functions.
-        source = source.replace("@autoreload\n", "")
+    source = source.replace("@autoreload\n", "").replace("@reloadr\n", "")
P6rguVyrst commented 1 year ago

@hoh If a regex approach is OK to remove all the decorators that follow @reloadr, then I can take this on me. I'd try to find time this week to work on it.

Screenshot 2023-02-08 at 08 23 42

I'd be looking to add a test for get_new_source function (I'm familiar with pytest, but maybe you prefer something else?), and maybe something like tox (maybe you have better recommendations) to test it with different python versions. Could add GHA to run the test and add new python versions to test with - to make sure future versions wouldn't break it, this function.

hoh commented 1 year ago

Hi @P6rguVyrst ,

The regex approach looks fine. It would be wise to also include the following names as aliases of that decorator:

Testing using Pytest sounds wise. Regarding the matrix of Python versions, either Tox or GitHub Actions seem fine. I would prefer the change that fixes this issue and a simple test in one PR, and the addition of Tox, GitHub Actions, ... in another since that is not entirely related.

P6rguVyrst commented 1 year ago

It would be wise to also include the following names as aliases of that decorator

Took me a while, but I think I see what you mean.

Do you mean that the solution should not be limited to @reloadr only, but cover any one of those interfaces that are currently supported by the library.

    decorator_scope = [
        "@reloadr",
        "@autoreload",
        "@reloadr.reloadr",
        "@reloadr.autoreload",
    ]

Makes it a more interesting problem, but wouldn't mind giving a go extending my initial solution and try to come up with a more complete solution.

Technically speaking something like r"(@reloadr|@autoreload)(.*)(?=class)" should cover any functions in the reloadr module has + the from reloadr import autoreload

raw2026 commented 1 year ago

Has any further bug-fix progress been made ? I had issues with python 3.10 and reloadr.