Hi Heinrich, On Thu, 20 Apr 2023 at 17:57, Heinrich Schuchardt <heinrich.schucha...@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.schucha...@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.schucha...@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.schucha...@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.aka...@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