[PATCH 1/1] test: fix pylint warning for capsule tests

[Simon Glass]

Hi Heinrich,

On Thu, 20 Apr 2023 at 17:57, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
>
>
> On 4/20/23 00:40, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 19 Apr 2023 at 18:30, Heinrich Schuchardt
> > <heinrich.schuchardt at canonical.com> wrote:
> >>
> >>
> >>
> >> On 4/19/23 03:45, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Fri, 14 Apr 2023 at 02:34, Heinrich Schuchardt
> >>> <heinrich.schuchardt at canonical.com> wrote:
> >>>>
> >>>> Fix pylint warnings like:
> >>>>
> >>>> * Class inherits from object
> >>>> * Missing module description
> >>>> * Missing class description
> >>>> * First line of comment blank
> >>>> * Superfluous imports
> >>>>
> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> >>>> ---
> >>>>    test/py/tests/test_efi_capsule/conftest.py    | 27 ++++--------
> >>>>    .../test_capsule_firmware_fit.py              | 35 ++++++++--------
> >>>>    .../test_capsule_firmware_signed_fit.py       | 41 ++++++++++---------
> >>>>    .../test_capsule_firmware_signed_raw.py       | 38 ++++++++---------
> >>>>    4 files changed, 65 insertions(+), 76 deletions(-)
> >>>>
> >>>> diff --git a/test/py/tests/test_efi_capsule/conftest.py b/test/py/tests/test_efi_capsule/conftest.py
> >>>> index 4879f2b5c2..0e5137de60 100644
> >>>> --- a/test/py/tests/test_efi_capsule/conftest.py
> >>>> +++ b/test/py/tests/test_efi_capsule/conftest.py
> >>>> @@ -2,30 +2,21 @@
> >>>>    # Copyright (c) 2020, Linaro Limited
> >>>>    # Author: AKASHI Takahiro <takahiro.akashi at linaro.org>
> >>>>
> >>>> -import os
> >>>> -import os.path
> >>>> -import re
> >>>> -from subprocess import call, check_call, check_output, CalledProcessError
> >>>> -import pytest
> >>>> -from capsule_defs import *
> >>>> +"""Fixture for UEFI capsule test
> >>>> +"""
> >>>>
> >>>> -#
> >>>> -# Fixture for UEFI capsule test
> >>>> -#
> >>>> +from subprocess import call, check_call, CalledProcessError
> >>>> +import pytest
> >>>> +from capsule_defs import CAPSULE_DATA_DIR, CAPSULE_INSTALL_DIR, EFITOOLS_PATH
> >>>>
> >>>>    @pytest.fixture(scope='session')
> >>>>    def efi_capsule_data(request, u_boot_config):
> >>>> -    """Set up a file system to be used in UEFI capsule and
> >>>> -       authentication test.
> >>>> -
> >>>> -    Args:
> >>>> -        request: Pytest request object.
> >>>> -        u_boot_config: U-boot configuration.
> >>>> +    """Set up a file system to be used in UEFI capsule and authentication test
> >>>> +    and return a ath to disk image to be used for testing
> >>>
> >>> Thanks for cleaning this up. I suppose with all the rounds of review
> >>> we got tired of worrying about the style. Also this probably predates
> >>> the pylint check.
> >>>
> >>> Can we please follow the style in the rest of the code? This should
> >>> have a heading line, with further notes after a blank line.
> >>>
> >>>>
> >>>> -    Return:
> >>>> -        A path to disk image to be used for testing
> >>>> +    request -- Pytest request object.
> >>>> +    u_boot_config -- U-boot configuration.
> >>>
> >>> Again the style is:
> >>>
> >>> Return:
> >>>      request (pytest.Request): Request to be processed
> >>>      u_boot_config (type): U-Boot configuration
> >>
> >> This seems to be some Google internal code style. We should stick to
> >> PEP257 (https://peps.python.org/pep-0257/).
> >
> > That doesn't really tell you much though. It is just the idea of a
> > docstring from 20 years ago. We need a convention for what is in the
> > strings, not just the strings.
> >
> > The one we currently use is at least based on [1] or something like
> > it. I would not describe it as internal to Google. What makes you
> > think that? It is widely used.
> >
> > For me at least, pylint asks for type information:
> >
> > tools/binman/control.py:146:0: W9016: "modules, test_missing" missing
> > in parameter type documentation (missing-type-doc)
>
> You should add type hints (PEP484) and use snake case (PEP8):
>
> -def WriteEntryDocs(modules, test_missing=None):
> +def write_entry_docs(modules: List[str], test_missing: str=None):

PEP8 yes.

>
> -def ListEntries(image_fname, entry_paths):
> +def list_entries(image_fname: str, entry_paths: List[str]):

PEP484 OMG it is so horrible, and you have just shown some simple
cases. See tbot for a code base that uses this and it is not easy.

Regards,
Simon