[bug#45712] [PATCHES] Improve Python package quality

[Hartmut Goebel]

Hi Lars,

thanks for the patch set. Please please find some comments. (I did not
check all changes to the packages, assuming you did it right :-)


> +(define validate-script
> +  "from __future__ import print_function # Python 2 support.

Please but this int a new line - makeing it easier to copy & paste.
(Leading emptry lines doe nor effect "from __future__ import").

> +import pkg_resources, sys, importlib, traceback
> +try:
> +    from importlib.machinery import PathFinder
> +except ImportError:
> +    PathFinder = None



> +ws = pkg_resources.find_distributions(sys.argv[1])
> +for dist in ws:
> +    print('validating', repr(dist.project_name), dist.location)
> +    try:
> +        print('...checking requirements', end=': ')

This looks very uncommon (and make my mind hooking on it). Please use
this, which is more common and less mindbogling ;-)

print('...checking requirements: ', end='')


> +        req = str(dist.as_requirement())
> +        # dist.activate() is not enough to actually check requirements, we 
> have to
> +        # .require() it.
> +        pkg_resources.require(req)

Any reason for converting the req into a string first? IC,
"`requirements` must be a string". So I'd move the "str()" to the
function call:

  req = dist.as_requirement()
  # dist.activate() is not enough to actually check requirements,
  # we have to .require() it.
  pkg_resources.require(str(req))


> +        print('OK')
> +    except Exception as e:
> +        print('ERROR:', req, e)
> +        ret = 1
> +        continue
> +    # Try to load entry points of console scripts too, making sure they 
> work. They
> +    # should be removed if they don’t. Other groups may not be safe, as they 
> can
> +    # depend on optional packages.
> +    for group, v in dist.get_entry_map().items():
> +       if group not in {'console_scripts', }:

    if group not in {'console_scripts', 'gui_scripts'}:

Why not gui-scripts?
If not adding gui-scripts, just use "if group != 'concolse_scrips':"

> +           continue
> +       for name, ep in v.items():
> +           try:
> +               print('...trying to load endpoint', group, name, end=': ')

Here it is fine ;-)

> +    # And finally try to load top level modules. This should not have any
> +    # side-effects.

Does it make sence to try loading the top-level modules *after* the the
entry-point? Chances are very high that the entry-points implicitly test
the top-level modules already.

IMHO it would make more sense to first try the top-level modules, and
even stop processing if this fails.

> +    for name in dist.get_metadata_lines('top_level.txt'):
> +        # Only available on Python 3.
> +        if PathFinder and PathFinder.find_spec(name) is None:
> +            # Ignore unavailable modules. Cannot use

Please add a short explanation why there can be unavailable top-level
modules.


> +    (add-after 'check 'validate-loadable validate-loadable)

While not having antoher idea, I'm not happy with this name. "loadable"
is not easy to get. (See also below, where this term is used in commit message.)

top-level-modules-are-importable?

> +(define python-dummy-ok

AFAIKS the packages only differ by some requirements. So I suggest
using a function to return the packages. This makes it easier to spot
the actull differences.

> +           (replace 'unpack
> +             (lambda _
> +               (mkdir-p "src")
> +               (chdir "src")
> +               (mkdir-p "dummy")

(mkdir-p "src/dummy")
(chdir "src")



> +setup(
> +     name='dummy-fail-import',
> +     version='0.1',
> +     packages=['dummy'],
> +     )

I would strip this down (version is not required AFAIK) and put it one
line (her assuming you use (format) for creating the code within a function:

setup(name='~a', packages=~a, install_requires=~a)


> Subject: [PATCH 03/15] gnu: python-pytest-xdist: Add missing input, relax
>  pytest requirement.

> +         (add-after 'unpack 'patch
> +           (lambda* (#:key inputs #:allow-other-keys)

Arguments are not used?

> +             ;; Relax pytest requirement.
> +             (substitute* "setup.py"
> +               (("pytest>=6\\.0\\.0") "pytest"))
> +             #t))

Any reason for relaxing this? Why not use python-pytest-6 as input?

> Subject: [PATCH 04/15] gnu: python-fixtures-bootstrap: Do not apply loadable
>  check.

Please rephrase into something like

Do not validate loadability
Do not validate whetehr packag is loadable
…


> Subject: [PATCH 05/15] gnu: python-pytest-pep8: Fix package.

> -     `(#:tests? #f)) ; Fails with recent pytest and pep8. See upstream 
> issues #8 and #12.
> +     `(#:tests? #t ; Fails with recent pytest and pep8. See upstream issues 
> #8 and #12.

Dosn't this change fix the checks? So this comment would be obsoltes and
"#:tests #t" can be removed.


> Subject: [PATCH 06/15] gnu: python-pyfakefs: Disable unreliable test.

> -         (add-after 'check 'check-pytest-plugin
> +         (replace 'check
>             (lambda _
> -             (invoke
> -              "python" "-m" "pytest"
> -              "pyfakefs/pytest_tests/pytest_plugin_test.py")
> -             #t)))))
> +             (invoke "pytest"
> +               "pyfakefs/tests"
> +               ;; The default test suite does not run these extra tests.
> +               ;"pyfakefs/pytest_tests/pytest_plugin_test.py"
> +               ;; atime difference is larger than expected.
> +               "-k" "not test_copy_real_file"))))))

I suggest keeping the old way, as this is will automatically update to
upstream changes.