Code review comment for lp:~abentley/launchpad/bad-branch-name

Revision history for this message
Māris Fogels (mars) wrote :

Hi Aaron,

This change looks good. Some questions:

  * Would the text "No such branch: foo" be clearer for our users than "Unknown branch: foo"? I personally find the meaning of "No such X" clearer than "Unknown X".

  * I see two convoluted calls to "extract_text(find_tags_by_class(browser.contents, 'message')[1])" in this diff. Should it be extracted into a nice helper function like get_page_message()?

Otherwise, this looks good to me. r=mars

Maris

review: Approve

« Back to merge proposal