Re: [PATCH 1/1] test: fix pylint warning for capsule tests
Hi Heinrich, On Thu, 20 Apr 2023 at 17:57, Heinrich Schuchardt wrote: > > > > On 4/20/23 00:40, Simon Glass wrote: > > Hi Heinrich, > > > > On Wed, 19 Apr 2023 at 18:30, Heinrich Schuchardt > > wrote: > >> > >> > >> > >> On 4/19/23 03:45, Simon Glass wrote: > >>> Hi Heinrich, > >>> > >>> On Fri, 14 Apr 2023 at 02:34, Heinrich Schuchardt > >>> 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 > --- > 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 > > -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
Re: [PATCH 1/1] test: fix pylint warning for capsule tests
On 4/20/23 00:40, Simon Glass wrote: Hi Heinrich, On Wed, 19 Apr 2023 at 18:30, Heinrich Schuchardt wrote: On 4/19/23 03:45, Simon Glass wrote: Hi Heinrich, On Fri, 14 Apr 2023 at 02:34, Heinrich Schuchardt 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 --- 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 -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): -def ListEntries(image_fname, entry_paths): +def list_entries(image_fname: str, entry_paths: List[str]): Best regards Heinrich You should be able to try that and see what you get. Regards, Simon [1] https://sphinxcontrib-napoleon.readthedocs.io/en/latest/index.html
Re: [PATCH 1/1] test: fix pylint warning for capsule tests
Hi Heinrich, On Wed, 19 Apr 2023 at 18:30, Heinrich Schuchardt wrote: > > > > On 4/19/23 03:45, Simon Glass wrote: > > Hi Heinrich, > > > > On Fri, 14 Apr 2023 at 02:34, Heinrich Schuchardt > > 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 > >> --- > >> 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 > >> > >> -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 be able to try that and see what you get. Regards, Simon [1] https://sphinxcontrib-napoleon.readthedocs.io/en/latest/index.html
Re: [PATCH 1/1] test: fix pylint warning for capsule tests
On 4/19/23 03:45, Simon Glass wrote: Hi Heinrich, On Fri, 14 Apr 2023 at 02:34, Heinrich Schuchardt 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 --- 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 -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/). Best regards Heinrich (so avoid periods at the end and add the type in brackets) The u_boot_config thing is pretty annoying since it creates a strangely named class - see confest.py where is has: ubconfig = ArbitraryAttributeContainer() Really that should be a class with a sensible name and documented properties. The internals of this are a little too arcane for my liking and discoverability is not great. [..] Also, this should be added to the checker - try 'make pylint' to see that. Regards, Simon
Re: [PATCH 1/1] test: fix pylint warning for capsule tests
Hi Heinrich, On Fri, 14 Apr 2023 at 02:34, Heinrich Schuchardt 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 > --- > 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 > > -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 (so avoid periods at the end and add the type in brackets) The u_boot_config thing is pretty annoying since it creates a strangely named class - see confest.py where is has: ubconfig = ArbitraryAttributeContainer() Really that should be a class with a sensible name and documented properties. The internals of this are a little too arcane for my liking and discoverability is not great. [..] Also, this should be added to the checker - try 'make pylint' to see that. Regards, Simon
