Standardize output excerpt APIs in services.

[xpconanfan]

Part two of AndroidDevice output file improvements.

---

This change is 

[winterfroststrom]

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @mderu, @winterfroststrom, and @xpconanfan)

---

mobly/controllers/android_device_lib/service_manager.py, line 114 at r1 (raw file):

aliases = list(self._service_objects.keys())

nit. why not just iterate over this directly

---

mobly/controllers/android_device_lib/service_manager.py, line 131 at r1 (raw file):

            Dict, keys are the names of the services,
        """

this seems malformed and generally inconsistent with the rest of the code

---

mobly/controllers/android_device_lib/services/base_service.py, line 46 at r1 (raw file):

    @alias.setter
    def alias(self, alias):

docstring?

---

mobly/controllers/android_device_lib/services/base_service.py, line 118 at r1 (raw file):

String

this isn't consistent with the logcat change, also it's inconsistent with the rest of the code

---

tests/mobly/controllers/android_device_lib/services/logcat_test.py, line 273 at r1 (raw file):

t

s

[xpconanfan]

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @mderu, @winterfroststrom, and @xpconanfan)

---

mobly/controllers/android_device_lib/service_manager.py, line 114 at r1 (raw file):

Previously, winterfroststrom wrote…

aliases = list(self._service_objects.keys())

nit. why not just iterate over this directly

Bc people might modify the list during the iteration.
i eventually want to use this for all the iteration methods in this class..

---

mobly/controllers/android_device_lib/service_manager.py, line 131 at r1 (raw file):

Previously, winterfroststrom wrote…

            Dict, keys are the names of the services,
        """

this seems malformed and generally inconsistent with the rest of the code

Done.

---

mobly/controllers/android_device_lib/services/base_service.py, line 46 at r1 (raw file):

Previously, winterfroststrom wrote…

docstring?

do setters need docstrings since they are clearly associated with the property itself

---

mobly/controllers/android_device_lib/services/base_service.py, line 118 at r1 (raw file):

Previously, winterfroststrom wrote…

String

this isn't consistent with the logcat change, also it's inconsistent with the rest of the code

Done.

[xpconanfan]

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @mderu and @winterfroststrom)

---

tests/mobly/controllers/android_device_lib/services/logcat_test.py, line 273 at r1 (raw file):

Previously, winterfroststrom wrote…

t

s

Done.

[winterfroststrom]

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @mderu, @winterfroststrom, and @xpconanfan)

---

mobly/controllers/android_device_lib/service_manager.py, line 137 at r2 (raw file):

path

nit. paths since this is a list

---

mobly/controllers/android_device_lib/services/base_service.py, line 46 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

do setters need docstrings since they are clearly associated with the property itself

setters have docstrings in the rest of the codebase?

[xpconanfan]

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @mderu and @winterfroststrom)

---

mobly/controllers/android_device_lib/services/base_service.py, line 46 at r1 (raw file):

Previously, winterfroststrom wrote…

setters have docstrings in the rest of the codebase?

According to style guide, setters don't have docstrings. So I think we should follow that going forward

[winterfroststrom]

Reviewable status: 0 of 6 files reviewed, 6 unresolved discussions (waiting on @mderu and @xpconanfan)

---

mobly/controllers/android_device_lib/service_manager.py, line 114 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Bc people might modify the list during the iteration.
i eventually want to use this for all the iteration methods in this class..

add a unit test for that scenario then?

---

mobly/controllers/android_device_lib/service_manager.py, line 85 at r3 (raw file):

service_obj.alias = alias

this feels mildly errorprone to me because you could have an unregistered service that runs into problems with this (e.g. this is technically an optional value) and because this couples the services with the android controller class

---

mobly/controllers/android_device_lib/service_manager.py, line 138 at r3 (raw file):

service.alias

is this really necessary, why not just make the function take the alias and the service?

---

mobly/controllers/android_device_lib/services/base_service.py, line 23 at r3 (raw file):

_alias = None

so, what happens when you have multiple of the same service on different aliases?

---

mobly/controllers/android_device_lib/services/logcat.py, line 130 at r3 (raw file):

l

should be capitalized like the other usage?

---

tests/mobly/controllers/android_device_lib/service_manager_test.py, line 141 at r3 (raw file):

service1.ha.side_effect = Exception('Failure in service1.')

assert on the error actually being recorded instead of swallowed?

---

tests/mobly/controllers/android_device_lib/service_manager_test.py, line 155 at r3 (raw file):

['path/to/1.txt']

tests where the service returns multiple or no paths?

[xpconanfan]

Reviewable status: 0 of 6 files reviewed, 6 unresolved discussions (waiting on @mderu and @winterfroststrom)

---

mobly/controllers/android_device_lib/service_manager.py, line 85 at r3 (raw file):

Previously, winterfroststrom wrote…

service_obj.alias = alias

this feels mildly errorprone to me because you could have an unregistered service that runs into problems with this (e.g. this is technically an optional value) and because this couples the services with the android controller class

what would the problem be?

---

mobly/controllers/android_device_lib/service_manager.py, line 138 at r3 (raw file):

Previously, winterfroststrom wrote…

service.alias

is this really necessary, why not just make the function take the alias and the service?

The service should know what alias it's registered with.
Otherwise we'd have to pass two values at all times, which is cumbersome.

---

mobly/controllers/android_device_lib/services/base_service.py, line 23 at r3 (raw file):

Previously, winterfroststrom wrote…

_alias = None

so, what happens when you have multiple of the same service on different aliases?

what's the problem?

---

mobly/controllers/android_device_lib/services/logcat.py, line 130 at r3 (raw file):

Previously, winterfroststrom wrote…

l

should be capitalized like the other usage?

Done.

---

tests/mobly/controllers/android_device_lib/service_manager_test.py, line 141 at r3 (raw file):

Previously, winterfroststrom wrote…

service1.ha.side_effect = Exception('Failure in service1.')

assert on the error actually being recorded instead of swallowed?

Done.

---

tests/mobly/controllers/android_device_lib/service_manager_test.py, line 155 at r3 (raw file):

Previously, winterfroststrom wrote…

['path/to/1.txt']

tests where the service returns multiple or no paths?

Done.

[winterfroststrom]

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @mderu and @xpconanfan)

---

mobly/controllers/android_device_lib/services/base_service.py, line 23 at r3 (raw file):

Previously, xpconanfan (Ang Li) wrote…

what's the problem?

nvm, I misunderstood how python variables worked in this case, thought it would set the class's variable instead of the instance's

---

mobly/controllers/android_device_lib/services/base_service.py, line 42 at r4 (raw file):

        """Alias used to register this service with service manager."""
        return self._alias

nit. mention that this can be None if the service is used directly

[xpconanfan]

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @mderu and @winterfroststrom)

---

mobly/controllers/android_device_lib/services/base_service.py, line 23 at r3 (raw file):

Previously, winterfroststrom wrote…

nvm, I misunderstood how python variables worked in this case, thought it would set the class's variable instead of the instance's

Done.

[mderu]

Reviewed 1 of 6 files at r1, 1 of 3 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @winterfroststrom and @xpconanfan)

---

mobly/controllers/android_device_lib/services/base_service.py, line 111 at r5 (raw file):

Quoted 4 lines of code…

        For services that generates output files, calling this method would
        create excerpts of the output files. An excerpt should contain info
        between two calls of `create_output_excerpts` or from the start of the
        service to the call to `create_output_excerpts`.

nit: I'd add after this what the standard format of output file names are. That way, it's more clear what the expected format is without having to go to doc pages or read other output_excerpt implementations. The convention only works well if everyone buys into it, and it's easier to propagate if the convention is documented here as well.