Code review comment for lp:~allanlesage/address-book-app/abook_navigation_favorites

Revision history for this message
Leo Arias (elopio) wrote :

37 +#!/usr/bin/env python

I think you don't need this, as this file will never be executed as a script.

190 + @autopilot_logging.log_action(logger.info)

This log is unnecessary, as you you are already logging the action when calling cancel or delete.
What I would do is turn this:

191 + def click_button(self, objectname):

into _click_button, so we know it shouldn't be used outside the class. If we need to click another button, we will need to create a method for it.

214 + def get_contact_by_name(self,

I would rename this to _get_contact_label_by_name.
With the current name, it sounds like something that a test would call to get the text inside the label showing the contact name.

238 + raise StateNotFoundError('Label')
239 + except StateNotFoundError:
240 + logger.error('...')
241 + raise

I would prefer here to return our own exception, to differentiate it from something that autopilot throws.

Something like:
raise AddressBookError('Contact {} not found.'.format(contact_name))

I don't want to delay your branch anymore. It's already really nice. I have some comments that I think would make it a little more readable, but I'll make a note to discuss with you about them next week.

Thanks a lot, nice work.

review: Approve (code review)

« Back to merge proposal