Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update docstrings #7374

Merged
merged 13 commits into from May 2, 2017

Conversation

Projects
None yet
8 participants
@fabaff
Copy link
Member

commented Apr 30, 2017

Description:

Preparation for #7357.

Checklist:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
@mention-bot

This comment has been minimized.

Copy link

commented Apr 30, 2017

@fabaff, thanks for your PR! By analyzing the history of the files in this pull request, we identified @MartinHjelmare, @rytilahti and @olimpiurob to be potential reviewers.

homeassistant/components/media_player/plex.py Outdated
return self._media_album_artist

@property
def media_track(self):
"""Track number of current playing media, music track only."""
"""Return the track number of current playing media, music track only."""

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2017

line too long (81 > 79 characters)

homeassistant/components/media_player/onkyo.py Outdated
else:
_LOGGER.info('%s is disconnected. Attempting to reconnect.',
_LOGGER.info(""%s is disconnected. Attempting to reconnect"",

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2017

missing whitespace around modulo operator
SyntaxError: invalid syntax

homeassistant/components/media_player/emby.py Outdated
@@ -85,20 +85,20 @@ def device_update_callback(data):
new_devices.append(new)

elif dev_id in inactive_emby_devices:
if emby.devices[dev_id].state != 'Off':
if Callbackemby.devices[dev_id].state != 'Off':

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2017

undefined name 'Callbackemby'

homeassistant/components/media_player/plex.py Outdated
return self._media_album_artist

@property
def media_track(self):
"""Track number of current playing media, music track only."""
"""Return the track number of current playing media, music track only."""

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2017

line too long (81 > 79 characters)

homeassistant/components/media_player/onkyo.py Outdated
else:
_LOGGER.info('%s is disconnected. Attempting to reconnect.',
_LOGGER.info(""%s is disconnected. Attempting to reconnect"",

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2017

missing whitespace around modulo operator
SyntaxError: invalid syntax

homeassistant/components/media_player/emby.py Outdated
@@ -85,20 +85,20 @@ def device_update_callback(data):
new_devices.append(new)

elif dev_id in inactive_emby_devices:
if emby.devices[dev_id].state != 'Off':
if Callbackemby.devices[dev_id].state != 'Off':

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2017

undefined name 'Callbackemby'

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Apr 30, 2017

I'd like us to decide if we should have double quotes on all log strings or not. It's not part of PEP8 as far as I know but if home assistant makes it its standard we can all follow that. Either we make it standard or we should stop changing it. I've been following whole module consistency, but not whole project consistency. I don't care what we decide, but we need to have a standard to follow.

homeassistant/components/mqtt/server.py Outdated
@@ -1,4 +1,4 @@
"""
"""

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2017

IndentationError: unexpected indent
indentation is not a multiple of four
unexpected indentation

homeassistant/components/sensor/octoprint.py Outdated
SENSOR_TYPES[octo_type][1],
tool)
new_sensor = OctoPrintSensor(
octoprint.OCTOPRINT, temp_type,temp_type, name,

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2017

missing whitespace after ','

homeassistant/components/sensor/knx.py Outdated
ILLUMINANCE, SPEED_MS, CONF_MINIMUM,
CONF_MAXIMUM)
from homeassistant.const import (
TEMP_CELSIUS, TEMPERATURE, CONF_TYPE,ILLUMINANCE, SPEED_MS, CONF_MINIMUM,

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2017

missing whitespace after ','

homeassistant/components/switch/broadlink.py Outdated
CONF_TYPE)
from homeassistant.const import (
CONF_FRIENDLY_NAME, CONF_SWITCHES, CONF_COMMAND_OFF, CONF_COMMAND_ON,
ONF_TIMEOUT, CONF_HOST, CONF_MAC,CONF_TYPE)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2017

missing whitespace after ','

CONF_COMMAND_OFF, CONF_COMMAND_ON,
CONF_TIMEOUT, CONF_HOST, CONF_MAC,
CONF_TYPE)
from homeassistant.const import (

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 30, 2017

'homeassistant.const.ONF_TIMEOUT' imported but unused

homeassistant/components/binary_sensor/netatmo.py Outdated
"Outdoor motion": 'motion',
"Outdoor human": 'motion',
"Outdoor animal": 'motion',
"Outdoor vehicle": 'motion'

This comment has been minimized.

Copy link
@rytilahti

rytilahti Apr 30, 2017

Contributor

Converting only the value part doesn't make much sense, does it?

homeassistant/components/light/yeelight.py Outdated
name = "yeelight_%s_%s" % (discovery_info["device_type"],
discovery_info["properties"]["mac"])
name = 'yeelight_{}_{}'.format(
discovery_info['device_type'], discovery_info['properties']['mac'])

This comment has been minimized.

Copy link
@rytilahti

rytilahti Apr 30, 2017

Contributor

Imo format string changes should be discussed separately. In the future (with 3.6) it'd be nice to use f-strings directly, however for now I don't see what good does this change do (when not naming the placeholders, that is)?

This comment has been minimized.

Copy link
@fabaff

fabaff May 1, 2017

Author Member

If we suggest to use string formatting in PR then we should be consistent.

@@ -85,7 +84,7 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
device = {'name': device_config[CONF_NAME], 'ipaddr': ipaddr}
lights.append(YeelightLight(device, device_config))

add_devices(lights, True) # true to request an update before adding.
add_devices(lights, True)

This comment has been minimized.

Copy link
@rytilahti

rytilahti Apr 30, 2017

Contributor

This is there to remind that add_devices() has a magic boolean flag. It may also be useful for developers looking how others have done adding. In any case I think this kind of cleanups would deserve their own PR & discussion.

This comment has been minimized.

Copy link
@fabaff

fabaff May 1, 2017

Author Member

That's covered in the docs. No need to have that on a single place in the code.

This comment has been minimized.

Copy link
@rytilahti

rytilahti May 1, 2017

Contributor

Very implicitly, but can be fixed with a documentation update. Maybe enforcing using kwarg for booleans as a general policy? Would turn it to add_devices(lights, update_before_adding=True) to make it clear?

homeassistant/components/media_player/cast.py Outdated
@@ -204,7 +204,7 @@ def media_track(self):

@property
def media_series_title(self):
"""The title of the series of current playing media (TV Show only)."""
"""Return te title of the series of current playing media (TV only)."""

This comment has been minimized.

Copy link
@rytilahti

rytilahti Apr 30, 2017

Contributor

Typo

@@ -101,15 +101,15 @@ def _setup_sources(self, telnet):
@classmethod
def telnet_request(cls, telnet, command, all_lines=False):
"""Execute `command` and return the response."""
_LOGGER.debug('Sending: "%s"', command)
_LOGGER.debug("Sending: %s", command)

This comment has been minimized.

Copy link
@rytilahti

rytilahti Apr 30, 2017

Contributor

Here changing from '' to "" instead of other way around? It'd however be nice to keep the outputted command quoted as it was.

track_time_change(
hass, self.update, second=config.get(CONF_SECOND),
minute=config.get(CONF_MINUTE), hour=config.get(CONF_HOUR),
day=config.get(CONF_DAY))

This comment has been minimized.

Copy link
@rytilahti

rytilahti Apr 30, 2017

Contributor

The existing one looks to me much more readable with one line per kwarg.

@fabaff fabaff referenced this pull request May 1, 2017

Merged

Updated docstrings #7383

@balloob

This comment has been minimized.

Copy link
Member

commented May 1, 2017

I would prefer if we keep cleanups to the minimum necessary to get the upgrade of pydocstyle or pylint going. By updating other code, we can a) introduce bugs and b) mask author of code in git blame

@fabaff

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

I would prefer if we keep cleanups to the minimum necessary to get the upgrade of pydocstyle or pylint going.

Me too but as I needed to edit almost every file it was only efficient to update the most obvious and minor things we want have changed in PRs like string formatting and the removal of blank lines or update of comments was a side effect.
People will always have a different opinions about quotes, style, line length, init systems, or what every. To come back to the log messages and the quotes: The most used style won. As it's perhaps not the best/smartest/nicest way to do it but now it's on the way to be consistent across the whole code.

The format of the log messages is a different topic and for another day. It wouldn't harm to have a more uniform style as we know for sure that people are feeding the log to their tools.

fabaff added some commits Apr 30, 2017

fabaff added some commits Apr 30, 2017

@fabaff

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

Ok, I collected the things which always lead to discussions and create a page with details about the style.

@rytilahti

This comment has been minimized.

Copy link
Contributor

commented May 1, 2017

As I don't know where this would be best discussed, just adding my last comment regarding to the quote styling. Having two types of quotes makes it very hard to keep it consistent on the long run if the content of those variables may live (e.g. are not just constants). If this is to be enforced it would be nice to describe in the style guide why it makes sense to do so.

P.S. about the quotations in log messages, using them makes it (in my opinion) easier, not harder to parse for both people and with regular expressions. A fully structured logging format would be the best but it may be hard to pull consistently.

P.P.S. In the file header example, "For more details about this platform, please refer to the documentation at" crosses the line length limit of 72 for docstrings as defined in PEP8.

Just my 0.02e :-)

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented May 1, 2017

Good with the page. In my opinion mixing single and double quotes and having different style depending on the length of the string, makes for a harder to follow style. I'd prefer it to be clear and simple. Eg either use single quotes in whole module or double quotes in whole module.

@balloob

This comment has been minimized.

Copy link
Member

commented May 2, 2017

I agree with @fabaff , thanks for adding the page 👍

@balloob balloob merged commit a4f1f6e into home-assistant:dev May 2, 2017

4 checks passed

cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0006%) to 93.459%
Details
hound No violations found. Woof!

@fabaff fabaff deleted the fabaff:prepare-pep257 branch May 2, 2017

@balloob balloob referenced this pull request May 5, 2017

Merged

0.44 #7444

@home-assistant home-assistant locked and limited conversation to collaborators Aug 12, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.