Code review comment for lp://staging/~lucio.torre/txstatsd/add-config-file

Looks great, with some whitespace cleanup and naming consistency
issues fixed. +1.

[1] pep8 isn't supper happy. :)

=== pep8 ===

txstatsd/tests/test_service.py:15:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:19:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:26:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:30:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:37:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:38:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:39:9: E303 too many blank lines (2)
txstatsd/tests/test_service.py:40:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:41:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:42:5: E303 too many blank lines (2)
txstatsd/tests/test_service.py:47:55: E202 whitespace before ']'
txstatsd/tests/test_service.py:49:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:59:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:63:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:71:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:79:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:85:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:90:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:91:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:96:61: W292 no newline at end of file
twisted/plugins/txstatsd_plugin.py:22:38: W292 no newline at end of file
txstatsd/service.py:12:1: E302 expected 2 blank lines, found 1
txstatsd/service.py:16:1: W293 blank line contains whitespace
txstatsd/service.py:18:1: W293 blank line contains whitespace
txstatsd/service.py:24:1: W293 blank line contains whitespace
txstatsd/service.py:30:1: W293 blank line contains whitespace
txstatsd/service.py:36:1: W293 blank line contains whitespace
txstatsd/service.py:38:1: W293 blank line contains whitespace
txstatsd/service.py:43:1: W293 blank line contains whitespace
txstatsd/service.py:48:1: W293 blank line contains whitespace
txstatsd/service.py:58:1: W293 blank line contains whitespace
txstatsd/service.py:60:1: W293 blank line contains whitespace
txstatsd/service.py:61:1: W293 blank line contains whitespace
txstatsd/service.py:76:1: W293 blank line contains whitespace
txstatsd/service.py:77:1: W293 blank line contains whitespace
txstatsd/service.py:80:1: W293 blank line contains whitespace
txstatsd/service.py:94:1: W293 blank line contains whitespace
txstatsd/service.py:95:19: W292 no newline at end of file
setup.py:30:1: E302 expected 2 blank lines, found 1

[2] some warnings from pyflakes but you can ignore those.

=== pyflakes ===

setup.py:11: 'setuptools' imported but unused
setup.py:14: redefinition of unused 'find_packages' from line 12

[3] is 'refresh_plugin_cache' needed in setup.py? A docstring
explaining why would be nice.

[4] Please use assertEqual(expected, got) instead of assertEquals(got, expected)

[5] The docstring for test_no_config_option is the same as
test_set_parameter, please update it. Same in test_reads_from_config,
test_cmdline_overrides_config,

[6] We should decide between 'Statsd' and 'StatsD'. Some of the new
code you added uses the former, while the existing code uses the
latter. I think we should go with the latter.

[7] 'TxStatsdServiceMaker' could be named 'StatsDServiceMaker' and the
tapname could be simply 'statsd'. Since this requires 'twistd' to run,
it's pretty implicit that it's tx-based, so we might as well omit the
'tx' prefix.

[8] Is it really necessary to reach out to __class__ here? Seems like
a workaround for a bug. Either way it needs a comment explaining why.

+ optParameters = []
+
+ def __init__(self):
+ self.glue_defaults = {}
+ self.__class__.optParameters = []

[9] s/setting/settings/
     s/txstatsd/txStatsD/

+ The set of configuration setting for txstatsd.

[10] s/statsd/txStatsD?

+ """Create a statsd service."""

[11] s/txstatsd/statsd?

+ service.setName("txstatsd")

[12] Indentation seems to be off here:

+ listener = UDPServer(options["listen-port"],
+ StatsDServerProtocol(processor))

[13] Could you rename 'gparam' to 'glue_parameters' here please?

+ def get_file_parser(self, gparam=None, **kwargs):

[14] Please add docstrings to test_support_default_not_in_config,
test_ensure_config_values_coerced and test_service.

[15] I suggest using the service name as the section name in the
configuration file, by having serviceName = 'statsd' as a class
attribute in OptionsGlue, and then using 'self.serviceName' instead of
the hardcoded 'main' below:

+ result = self._config_file.get("main", item)