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

Add ObjCProtocol class #76

Merged
merged 12 commits into from
Nov 3, 2017
Merged

Conversation

dgelessus
Copy link
Collaborator

Implements parts of #69.

This adds an ObjCProtocol class similar to ObjCClass, which supports basic introspection (name and superprotocols). ObjCClasses now also have a protocols property to look up which protocols are adopted.

Creating custom protocols and adopting protocols in Python is not yet supported, I'm working on that next.

@dgelessus
Copy link
Collaborator Author

OK, this is mostly done now. At the moment flake8 complains about ObjCClass.__new__ being too complex, because of the added logic for adopting protocols. I'll see if I can do something about that.

@dgelessus dgelessus changed the title Add ObjCProtocol class (WIP) Add ObjCProtocol class Oct 18, 2017
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

As always, very little to fault here from a technical perspective.

One thing I'd like to see before we merge is some documentation. We haven't historically required docs before merging new features, but we need to get on top of this before we've got a monster project with no docs.

I've just addressed the review notes on #67; if we can get that merged in, you can add a quick set of explanatory notes as a new how-to document, and we can merge this in!

results = {}

class Handler(NSObject):
class Handler(NSObject, protocols=[Callback]):
Copy link
Member

Choose a reason for hiding this comment

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

Nice usage of metaclass kwargs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, they are really useful for this sort of thing. I didn't want to do it this way originally - first I tried to put the protocols directly in the bases list (after the base class), but Python didn't like that, because the superclass has a different metaclass (ObjCClass) than the protocols (ObjCProtocol). In the end this way is probably best though, it's easier to read and cleaner to implement.

@dgelessus
Copy link
Collaborator Author

Agreed about the documentation - I didn't really think much about that yet, but it is a good idea to document new features right away.

@dgelessus
Copy link
Collaborator Author

Alright, I finally got this done, sorry for the delay - real life was a bit busier than expected.

I'm not sure why the Travis build hasn't started yet, but tox ran without issues for me locally, so everything should be fine.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

:shipit:

@freakboy3742 freakboy3742 merged commit 500f431 into beeware:master Nov 3, 2017
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