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

actually hook up the unknown capability fallback #191

Merged
merged 4 commits into from
Dec 6, 2014

Conversation

Habbie
Copy link
Contributor

@Habbie Habbie commented Dec 5, 2014

Without this patch, _fallback_capability is set only on the UnknownCapability class, not on the Capability class.

To clarify, the very similar looking register_capability actually works because all subclasses share the dict that is attached to registered_capability, and no assignment to it happens, just operations on it. However, fallback happens via assignment, and hence we need this patch.

@Habbie
Copy link
Contributor Author

Habbie commented Dec 5, 2014

(You can merge it like this but I will try to add a test for this situation)

@Habbie
Copy link
Contributor Author

Habbie commented Dec 5, 2014

Test added! It is a bit hacky though.

@thomas-mangin
Copy link
Member

Ok, I understand your patch, I have no issue to merge it, however the test code for capability 66 makes no sense to me. Can you please enlighten me 😄

@thomas-mangin
Copy link
Member

Got it ! Damn, I am slow tonight ! Could you:

  • add some credit for your work in the CHANGELOG file
  • rename !open-sends-cap66 to something like option:open:send-unknown-capbility (without the ! using two : mean that the normal parsing can be done).
    I will then merge

@thomas-mangin
Copy link
Member

Today is not my day ... if the function is not a classmethod, it should be made a classmethod which take the cls as first parameter. so the patch should only be to change @classmethod to @staticmethod (obviously the call must be changed as you did).

@Habbie
Copy link
Contributor Author

Habbie commented Dec 6, 2014

I don't follow - Capability.fallback_capability() is a classmethod, before and after my patch. If we make it a staticmethod, it no longer gets the cls argument.

@Habbie
Copy link
Contributor Author

Habbie commented Dec 6, 2014

changelog & option rename done. I should probably squash all of this but I'll wait until we are happy with the total result :)

@@ -52,10 +52,10 @@ def hex (data):
return '0x' + ''.join('%02x' % ord(_) for _ in data)

@classmethod
def fallback_capability (cls):
Copy link
Member

Choose a reason for hiding this comment

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

really should just change the @classmethod to @staticmethod

@thomas-mangin
Copy link
Member

Yes, it you could squash all the commit in one, once the @staticmethod change is perform, I will pull.
Thank you for your help.

@thomas-mangin
Copy link
Member

/me bad. reading the patch and not the patched file ...

thomas-mangin added a commit that referenced this pull request Dec 6, 2014
Do not drop session when receiving an unknown capability
@thomas-mangin thomas-mangin merged commit 553ee7e into Exa-Networks:master Dec 6, 2014
@Habbie Habbie deleted the unknowncap branch December 30, 2014 19:18
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.

2 participants