https://bugzilla.redhat.com/show_bug.cgi?id=2263790 --- Comment #35 from Miro Hrončok <mhroncok@xxxxxxxxxx> --- OK. MY highly opinionated feedback: > %global srcname yq This is unnecessary and makes the specfile harder to read. It would be easier to read if all occurrences of %{srcname} were replaced with yq. > %global srcforge https://github.com/kislyuk/yq So is this. It would be easier to read if the single occurrence of %{srcforge} was replaced with https://github.com/kislyuk/yq > URL: https://pypi.org/project/yq/ This is not the URL of the project, it is merely a URL of the package on PyPI. Upstream says their homepage is https://github.com/kislyuk/yq > Patch1: yq-tomlkit-f39.patch There is no reason to number patches. The patch has no comment that explains why it is needed. The patch has no commit message either. The name is not obvious enough. We usually use sed for trivial changes like this. > %if 0%{?fedora} && 0%{?fedora} <= 39 > BuildRequires: sed > %endif There is no reason to conditionalize this BuildRequires, it makes the specfile needlessly complicated. It is always present, so if you want to be explicit, just BuildRequire it unconditionally; if you want to be concise, don't BuildRequire it at all. > %global _description %{expand: > ...} > > %description > %{_description} This is inserting an undesired newline at the beginning of the description. Put %description %{_description} on a single line to avoid that. > %{?python_provide:%python_provide python3-%{srcname}} %python_provide is unnecessary and deprecated. The provides are generated automatically without it. > # Until Go yq provides alternatives, conflict with that package > Conflicts: yq If there are no alternatives in yq, don't add them here. Or coordinate. But don't have it halfway. This way it adds all the disadvantages of alternatives without the benefit of alternatives. > %if 0%{?fedora} && 0%{?fedora} <= 39 > ... > %autopatch -M 1 -p1 > %endif There is no reason to apply this patch conditionally. Use %autosetup without -N. > %generate_buildrequires Other sections are separated by 2 newlines. This section is separated by 1 newline. It seems inconsistent. > # xq command is already taken > # ... > mv %{buildroot}%{_bindir}/{xq,xqp} > mv %{buildroot}%{_bindir}/{yq,yqp} Why xqp, why yqp? The comment should try to explain that. > touch %{buildroot}%{_bindir}/yq Why use alternatives to this command only? > # python3 setup.py test is failing. Run test directly. Moreover, setup.py test is deprecated, so not using it is the right thing to do. > %license LICENSE The %license is already included in %{pyproject_files}, see: $ rpm -ql --licensefiles -p python3-yq-3.4.3-12.fc41.noarch.rpm /usr/lib/python3.13/site-packages/yq-3.4.3.dist-info/LICENSE /usr/share/licenses/python3-yq/LICENSE It should not be manually repeated in the %files section. Add -l flag to %pyproject_save_files instead to assert the %license is there even in newer versions. ---- General note about using alternatives: Are both tools' CLI APIs identical? If they are not, don't use alternatives. IF they ar identical: As a user, what is the benefit for me to use either version? If there is no difference, do I really need to have the option to choose? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2263790 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202263790%23c35 -- _______________________________________________ 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
