-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Replace in-place lists with tuples (1) #53684
Conversation
@@ -81,7 +81,7 @@ async def async_setup(hass, config): | |||
options = {} | |||
mode = conf.get(CONF_MODE, MODE_ROUTER) | |||
for name, value in conf.items(): | |||
if name in ([CONF_DNSMASQ, CONF_INTERFACE, CONF_REQUIRE_IP]): | |||
if name in (CONF_DNSMASQ, CONF_INTERFACE, CONF_REQUIRE_IP): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not be consistent? There are tuples ending with ,
and not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat special. The parentheses here weren't used before, i.e.
([CONF_DNSMASQ, CONF_INTERFACE, CONF_REQUIRE_IP]) == [CONF_DNSMASQ, CONF_INTERFACE, CONF_REQUIRE_IP]
There wasn't a trailing comma, so no tuple.
Take a look at the next line as well: if name == CONF_REQUIRE_IP ...
. That suggests that it should only be a normal list / tuple. Not a tuple of tuples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I took this as an example (bad one), but it's still mixed and matched everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You normally don't need a trailing comma for tuples. Only if there is just one value.
I won't recommend adding additional once either as this would change the formatting with black.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes for Shelly looks OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok for deconz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for Kodi and Sonos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for Braother and BraviaTV.
@@ -180,7 +180,7 @@ def __init__(self, name, device, entry, unique_id, coordinator): | |||
self._min_humidity = 30 | |||
self._max_humidity = 80 | |||
self._humidity_steps = 10 | |||
elif self._model in [MODEL_AIRHUMIDIFIER_CA4]: | |||
elif self._model in (MODEL_AIRHUMIDIFIER_CA4,): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better:
elif self._model == MODEL_AIRHUMIDIFIER_CA4:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bieniu The focus for this PR was to only replace lists with tuples. It doesn't really make sense to change one or two more these into equality checks. I might consider adding another pylint check for it to replace all of them at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good from philips_js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for rachio, recorder, sonos, and tado
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for roon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zwave looks good 👍🏾
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT for Shelly
As I wrote in #53686 (comment), I think the right change would be to use sets for lookups instead of lists or tuples. Those are designed and tuned precisely for this usage. |
I disagree with that. As performance wise, there is no real world difference, using tuples everywhere will make it easier for beginners to understand when to use them. For my full argument see: pylint-dev/pylint#4776 (comment) |
Closing this after discussion in pylint-dev/pylint#4776 |
Proposed change
Followup to #53147
Replace in-place defined lists with tuples.
Example
A check for it will be included with the next
pylint
release: pylint-dev/pylint#4768Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: