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

[1.4.0rc1] warning wrapper fails if wrapping an instance method #3348

Closed
foosel opened this issue Nov 26, 2019 · 4 comments

Comments

@foosel
Copy link
Owner

@foosel foosel commented Nov 26, 2019

Problem

2019-11-25 17:59:12,930 - octoprint.plugin - ERROR - Error while calling plugin fanspeedslider
Traceback (most recent call last):
  File "/home/nas/OctoPrint/venv/local/lib/python2.7/site-packages/octoprint/plugin/__init__.py", line 224, in call_plugin
    result = getattr(plugin, method)(*args, **kwargs)
  File "/home/nas/OctoPrint/venv/local/lib/python2.7/site-packages/octoprint_fanspeedslider/__init__.py", line 19, in on_after_startup
    self.get_settings_updates()
  File "/home/nas/OctoPrint/venv/local/lib/python2.7/site-packages/octoprint_fanspeedslider/__init__.py", line 90, in get_settings_updates
    self.defaultFanSpeed = self._settings.getInt(["defaultFanSpeed"])
  File "/home/nas/OctoPrint/venv/local/lib/python2.7/site-packages/octoprint/plugin/__init__.py", line 521, in __getattr__
    orig_func = decorator(orig_func)
  File "/home/nas/OctoPrint/venv/local/lib/python2.7/site-packages/octoprint/util/__init__.py", line 134, in decorator
    func.__qualname__ = to_native_str('warning_decorator_factory')
AttributeError: 'instancemethod' object has no attribute '__qualname__'

Also observed under Python 3 with AttributeError: 'method' object has no attribute '__qualname__'.

Problem only occurs when wrapping instance methods instead of functions, e.g. inside the deprecated method declarations in PluginSettings, hence why it is observable primarily with plugins still using deprecated settings methods (eg. getInt instead of get_int).

Same issue also happens with __annotations__.

Solution

Let functools.wraps just do its job?

Additional remarks

@smurfix I saw that you added the two lines that are causing issues here in 9e4b0f8 which sadly only said "misc". I'm unclear on why you added these two attribute assignments in the first place. As far as I understand it those lines should already be taken care of by functools.wraps or rather functools.update_wrapper:

WRAPPER_ASSIGNMENTS = ('__module__', '__name__', '__qualname__', '__doc__',
                       '__annotations__')
WRAPPER_UPDATES = ('__dict__',)
def update_wrapper(wrapper,
                   wrapped,
                   assigned = WRAPPER_ASSIGNMENTS,
                   updated = WRAPPER_UPDATES):
    """Update a wrapper function to look like the wrapped function

       wrapper is the function to be updated
       wrapped is the original function
       assigned is a tuple naming the attributes assigned directly
       from the wrapped function to the wrapper function (defaults to
       functools.WRAPPER_ASSIGNMENTS)
       updated is a tuple naming the attributes of the wrapper that
       are updated with the corresponding attribute from the wrapped
       function (defaults to functools.WRAPPER_UPDATES)
    """
    for attr in assigned:
        try:
            value = getattr(wrapped, attr)
        except AttributeError:
            pass
        else:
            setattr(wrapper, attr, value)
    for attr in updated:
        getattr(wrapper, attr).update(getattr(wrapped, attr, {}))
    # Issue #17482: set __wrapped__ last so we don't inadvertently copy it
    # from the wrapped function when updating __dict__
    wrapper.__wrapped__ = wrapped
    # Return the wrapper so this can be used as a decorator via partial()
    return wrapper

Do you remember the background here? Based on what I'm currently seeing, I would simply remove those lines again as they are causing issues and I don't even see why these two attributes are rewritten in the first place.

@foosel

This comment has been minimized.

Copy link
Owner Author

@foosel foosel commented Nov 26, 2019

@smurfix

This comment has been minimized.

Copy link

@smurfix smurfix commented Nov 26, 2019

Sorry, I can't remember what that code's usecase was.
Probably easiest to just remove them and check whether anything breaks. :-P

foosel added a commit that referenced this issue Nov 26, 2019
@foosel

This comment has been minimized.

Copy link
Owner Author

@foosel foosel commented Nov 26, 2019

I just figured it out ^^ Makes some tests fail. Ironically those tests don't actually test THAT. Looking into it

foosel added a commit that referenced this issue Nov 26, 2019
@foosel

This comment has been minimized.

Copy link
Owner Author

@foosel foosel commented Dec 2, 2019

1.4.0rc2 is out.

@foosel foosel closed this Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.