-
Notifications
You must be signed in to change notification settings - Fork 792
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
Disallow Screen
instances in App.SCREENS
#4894
Disallow Screen
instances in App.SCREENS
#4894
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some requests. I think the documentation may need an update. There are a few examples which use the old Screen instances in the classvar. There may also be some copy change required...
Should be all done now, thanks! |
I think all the places where an instance was used in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of requests then we should be good to go...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks!
Sorry; I'll fix the merge conflicts sometime later today. |
Merge conflicts have been fixed! |
Are you really 16?! I figured you were already a pro. |
Yup! I've been doing this since I was ~9. |
cc @willmcgugan
Closes #4893.
It's worth noting that while implementing this, I discovered a second issue:
add_mode
appends to theMODES
classvar, meaning it applies to all instances of theApp
-- if this is the desired behavior, then it should probably be made a classmethod. Otherwise, maybe an attribute to hold per-instance modes?