GNU bug report logs - #78322
[PATCH] Add Konsave

Previous Next

Full log

Message #29 received at 78322 <at> debbugs.gnu.org (full text, mbox):

From: Sergio Pastor Pérez <sergio.pastorperez <at> gmail.com>
To: Sharlatan Hellseher <sharlatanus <at> gmail.com>, 78322 <at> debbugs.gnu.org
Subject: Re: [PATCH] Add Konsave
Date: Fri, 09 May 2025 21:36:29 +0200

Sharlatan Hellseher <sharlatanus <at> gmail.com> writes:
> Hi,
>
> Thank you for the patch.
>
> Please check my review points and prepare updates in v2.

Done!

>> [PATCH] gnu: Add python-pytest-pylint.
> Place to python-check with
>
>   guix import -i gnu/packages/python-check.scm pypi pytest-pylint"
>
> It will import the package and save in the file in alphabetical order.

Sure.

>> [PATCH] gnu: Add konsave.
> Do you really want it in kde-utils as it requires to use quite a lot of
> new modules, how about python-xyz or python-web or even
> configuration-management (more likely according to description of the
> project)?
>
> From the upstream:
>> It officially supports KDE Plasma but it can be used on all other
>> desktop environments too!
> I'm not sure it may be a driving point to think it's just for KDE ;-)

Fair enough.

>> +    (native-inputs (list python-aiohttp
>> +                         python-aiohttp-cors
>> +                         python-black
>> +                         python-pylint
>> +                         python-setuptools
>> +                         python-wheel))
> No need for python-aiohttp python-aiohttp-cors python-black
> python-pylint, as tests are disabled and we usually ignore any dev
> dependencies (black, flake8, pylint and similar):
>
> setup.py:
>   -> install_requires=_REQUIREMENTS,
>     -> _REQUIREMENTS: List[str] = _read_reqs(Path("requirements.txt"))
>       -> PyYaml>=5.4.1

Removed.

>> +    (synopsis "Linux customization saver")
> It sounds miss leading as under "Linux customization" may be any sort of
> config files possible for Linux distros, according to farther description
> it's "dotfiles manager" so change to
> "Dotfiles manager" is more likely to reflect the purpose of the project.
>
>> +    "Konsave is command line program written in Python to let you backup your
>> +dotfiles and switch to other ones in an instant.  It officially supports KDE
>> +Plasma.")
> IMHO to mention which language the final CLI was written is ambiguous, I
> would reward it to something reflecting the purpose and a list of
> features, if remove "adetersment" we don't have much to have a clue what
> this CLI can do.

Right.

>> +    "Konsave is command line program .................... let you backup your
>> +dotfiles and switch to other ones in an instant.  ..........................
>> +.......")
>
>> +    (home-page "https://www.github.com/prayag2/konsave")
> change to "https://github.com/prayag2/konsave" as permament redirection

Done.

>> [PATCH] gnu: Add python-aiohttp-cors.
>
>> +    (native-inputs (list python-pytest-cov
>> +                         python-pytest-pylint
>> +                         python-pytest-runner
>> +                         python-selenium
>> +                         python-setuptools
>> +                         python-tox
>> +                         python-wheel))
> Pleas remove python-pytest-pylint, python-pytest-runner,
> python-selenium and python-tox; add python-pytest it will let
> pyproject-build-system to identify test runner automatically
>
>> +         (add-before 'check 'remove-network-dep
>> +           (lambda _
>> +             ;; Tests try to install from network even if its available
>> +             ;; locally.
>> +             (substitute* "setup.py"
>> +               (("\"aiohttp>=3.9\",") ""))))
>> +         ;; Tests run with `py.test' require a GUI browser window.
>> +         (replace 'check
>> +           (lambda* (#:key tests? #:allow-other-keys)
>> +             (if tests?
>> +                 (invoke "tox")
>> +                 (format #t "test suite not run~%"))
>> +             #t)))))
> You can drop this, see above comment.

I've removed the ones not needed but `python-tox' is required. The check
phase needs to be replaced to not run the testsuite that requires a
graphical display and a browser GUI.

>> +    (description
>> +     "This library implements Cross Origin Resource Sharing (CORS)
>> +support for aiohttp asyncio-powered asynchronous HTTP server.")
>> +    (license license:asl2.0)))
> You can use @acronym{} for styling.

I didn't know about that.

Thanks for the review! I've sent the revised patch series with the 3
patches, I guess adding `python-aiohttp-cors' is no longer necessary so
you can drop that patch if you don't like how it is packaged.


Best regards,
Sergio.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.