Subject: Re: [PATCH] Add Konsave

From: Sergio Pastor Pérez <[email-masked]>
Date: Fri, 09 May 2025 21:36:29 +0200

Sharlatan Hellseher <[email-masked]> 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.