Skip to content

Commit

Permalink
Properly revert _app.py callback to state before PR #442 (previously …
Browse files Browse the repository at this point in the history 
…only partially reverted)
  • Loading branch information
@engn33r
engn33r committed Feb 15, 2021
1 parent 960b715 commit 3112b7d
Showing 1 changed file with 1 addition and 4 deletions.
5 changes: 1 addition & 4 deletions websocket/_app.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -386,10 +386,7 @@ def _get_close_args(self, data):
def _callback(self, callback, *args):
if callback:
try:
if inspect.ismethod(callback):
callback(self, *args)
else:
callback(self, *args)
callback(self, *args)

except Exception as e:
_logging.error("error from callback {}: {}".format(callback, e))
Expand Down

6 comments on commit 3112b7d

@engn33r
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's my understanding of the history of this callback code, so that you can point out what details I'm missing:

  1. Aug 14 2018: PR Patch WebSocketApp class to make it inheritable #442 was merged and it tried to implement some inheritance features. Unfortunately, it broke many features and was reverted with ae58949.
  2. Aug 19 2018: The commit ae58949 only partially reverted Patch WebSocketApp class to make it inheritable #442, and bhy pointed out in this comment that the callback portion was not reverted.
  3. Sep 3 2018: PR Update _app.py #468 was merged, and it removed the "inspect.ismethod(callback)" branch that was added in PR Patch WebSocketApp class to make it inheritable #442. Unfortunately it didn't explain that it was completing the revert of PR Patch WebSocketApp class to make it inheritable #442 - if a clearer explanation has been given, the following events may not have occurred.
  4. Sep 5 2018: Project author liris responds to issue on_open() missing 1 required positional argument: 'ws' #471, discussing the recent callback edits, mentioning that the PR Update _app.py #468 merge was a mistake, as MRSharff suggests. This was around the time that the project stopped being maintained, so it is possible liris assumed MRSharff had a good understanding of this topic. Commit efcf31b returns the callback code to the way it was after PR Patch WebSocketApp class to make it inheritable #442. There is a long conversation about this commit
  5. Jan 9 2021: The new maintainer, @engn33r, merges PR fix callback #502 without a full understanding of the history of this issue
  6. Feb 14 2021: @engn33r commits 3112b7d to finally bring the callback code back to the way it was before PR Patch WebSocketApp class to make it inheritable #442

@massiou
Copy link

@massiou massiou commented on 3112b7d Mar 3, 2021

Choose a reason for hiding this comment

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

FYI it broke compatibility with Pysher 1.0.6 deepbrook/Pysher#62

@zhelbakov
Copy link

@zhelbakov zhelbakov commented on 3112b7d Mar 3, 2021 •
edited

Choose a reason for hiding this comment

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

It broke usage of WebSocketApp without inheritance:

class JsonWebsocket(object):
    def __init__(self, connection_string):
        self.__socket = websocket.WebSocketApp(connection_string, on_open=self._on_open)
    def _on_open(self):
        print("connection is opened")

Error: error from callback <bound method JsonWebsocket._on_open of <JsonWebsocket object at 0x000001FBAF9D90B8>>: _on_open() takes 1 positional argument but 2 were given

@Mips2648
Copy link

Choose a reason for hiding this comment

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

This is not actually this commit that broke it but this one: 40e3a93

@morpheus65535
Copy link

Choose a reason for hiding this comment

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

@engn33r hey there, I'm trying to make SignalRCore module work with websocket-client newer than 0.54.0 but having rolled back to how it was prior to #442 broke compatibility. Why not keep this code:

    def _callback(self, callback, *args):
        if callback:
            try:
                if inspect.ismethod(callback):
                    callback(*args)
                else:
                    callback(self, *args)

If I bring it back in 0.59.0, everything works perfectly (at least for the module I'm trying to upgrade.

Does it causes issues that I'm not aware of?

Thanks for maintaining this module btw!

@engn33r
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@morpheus65535 This is a known issue, see #669 and the duplicate issues referenced within. Release v0.58.0 introduced a breaking change and was not numbered following semver. The 1.0.0 release transitions the project to use semver properly.

Please sign in to comment.