Code review comment for ~lihow731/plainbox-provider-checkbox/+git/plainbox-provider-checkbox:master

Revision history for this message
Pierre Equoy (pieq) wrote :

Thanks for fixing this!

A few comments inline (see below).

Also, regarding the commit title/message:

- Usually, the commit title includes the area that is impacted. In this case, you can prefix your commit title with `bin/audio_test.py: `
- Your commit message includes a lot of info (which is great!) but it has a few typos and I think "In get_volume() and mute(), the self.method(command) returns a zero-length string." is not correct: the `get_volume()` method does not access `self.method(command)` (I guess you meant `set_volume()`?)
- You can include `LP: #1890526` (for instance, at the very bottom of the commit message) to automatically link a merge request in Launchpad with a given Launchpad bug[1]. It's also a good thing to have in a project history.

[1] https://help.launchpad.net/Code/Git#Linking_to_bugs

review: Needs Fixing

« Back to merge proposal