[Bug 2164874] Review Request: python-jaraco-stream - Routines for dealing with data streams

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=2164874



--- Comment #3 from Ondrej Mosnáček <omosnacek@xxxxxxxxx> ---
Thank you for the review!

(In reply to Ben Beasley from comment #2)
> - Patches no longer need to be numbered unless you are applying selected
>   patches by number. You can just write:
> 
>     Patch:          0001-Require-more_itertools.patch
>     Patch:          0002-Disable-linters.patch

Ah, good to know. I'll probably leave it as it is here, but I'll try to
remember it the next time I deal with patches :)

> - Instead of patching out the linters with 0002-Disable-linters.patch, you
>   might find it easier to simply add:
> 
>     BuildRequires:  python3dist(pytest)
> 
>   and drop “-t” from %pyproject_buildrequires, and then run the tests with
>   %pytest instead of %tox. However, the patch is a fine approach as long as
> it
>   is not too annoying to maintain.

Indeed for this simple package hard-coding the BuildRequires is simpler, but I
like to have the dependencies auto-generated from the metadata in case they
change in the future. For now I'll leave it as is, but might revisit the
approach later.

> 
>   If you do keep using the patch, it would be good to add the following
> comment
>   above it in the spec file as justification:
> 
>     #
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_linters

That's a good idea, I'll add it after importing the package.

> - Co-ownership of /usr/lib/python3.11/site-packages/jaraco is an appropriate
>   approach for Python namespace packages.
> 
>   However, in this case a package (python3-jaraco) exists solely to provide
>   that directory, so it might be better to depend on it instead:
> 
>     %package     -n python3-%{pkgname}
>     […]
>     Requires:       python3dist(jaraco)
> 
>     […]
> 
>     %files -n python3-%{pkgname} -f %{pyproject_files}
>     […]
>     # Owned by python3dist(jaraco)
>     %exclude %dir %{python3_sitelib}/jaraco
> 
>   This would be consistent with other existing Jaraco packages
>   (python-jaraco-{classes,collections,envs,functools,path}).

Good point, I'll also apply this suggestion.

> - I personally think the indirection of the modname, projname, and pkgname
>   macros makes the specfile harder rather than easier to read, and I would
> just
>   write out the appropriate strings where they appear. However, this is
> purely
>   a matter of taste.

I prefer it like this, mainly because it makes it easier to use the spec as a
base for new packages (copy-paste and change what's needed).

> - Since pyproject_files contains a properly marked license file in dist-info,
>   you can omit the manual one:
> 
>     %license LICENSE
> 
>   It’s always good to verify this as follows, since this doesn’t happen for
>   some Python build backends (like Poetry or flit-core) or in certain other
>   cases:
> 
>     $ rpm -qL -p results/python3-jaraco-stream-3.0.3-1.fc39.noarch.rpm
>     /usr/lib/python3.11/site-packages/jaraco.stream-3.0.3.dist-info/LICENSE
>     /usr/share/licenses/python3-jaraco-stream/LICENSE

Oh, good point. I'll apply this and hopefully rpmlint/rpminspect will complain
when run by Zuul if the file disappears :)


-- 
You are receiving this mail because:
You are always notified about changes to this product and component
You are on the CC list for the bug.
https://bugzilla.redhat.com/show_bug.cgi?id=2164874
_______________________________________________
package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux