Code review comment for lp:~mblayman/entertainer/translate-events

Revision history for this message
Samuel Buffet (samuel-buffet) wrote :

Matt,

I'm okay with the 2 goals of this branch.

This branch will lead me to change my callback variable name in "back_button" but there's no compatibility issues.
In "back_button" it's just handled on the Screen Class and wrapped in a "screen_back" method because I need that on every screen.

Bellow some minor comments :

=== modified file 'entertainerlib/frontend/gui/screen_history.py'
--- entertainerlib/frontend/gui/screen_history.py 2009-05-06 03:40:22 +0000
+++ entertainerlib/frontend/gui/screen_history.py 2009-06-02 01:16:03 +0000
@@ -1,69 +1,35 @@
 # Copyright (c) 2009 Entertainer Developers - See COPYING - GPLv2
 '''Stack container for screen objects'''

> def is_empty(self):
>- """
>- Is screen history empty.
>- @return boolean value. True if history is empty, otherwise False.
>- """
>+ '''Is screen history empty'''
> if len(self.screens) == 0:
> return True
> else:
> return False
>

Maybe a @property makes sense here instead of a method call to fit with the recent changes we've made.
The docstring needs a full stop.

Thanks,
Samuel-

review: Approve

« Back to merge proposal