https://bugzilla.redhat.com/show_bug.cgi?id=2117112 --- Comment #5 from Jonathan Wright <jonathan@xxxxxxxxxxxxx> --- (In reply to Miro Hrončok from comment #4) > Further feedback, as I was the original reviewer for bz2090791 but then got > cut off by Nick without involving me here :( I started on the package before realizing he had one pending review. We chatted on IRC and I offered him my spec but he said just submit it and he'd review. Sorry about that. > > rm -rf src/%{name}.egg-info > > This is most likely not needed and should not be in the spec file just > because some older packages have it. Removed. Came across the specific docs on that today actually. > > Source0: %{pypi_source} > > Using %{pypi_source} without the name argument is deprecated. Added the name. > > URL: https://pypi.python.org/pypi/%{pypi_name} > > Two things here: > > 1. the URL cannot be copied from the spec and pasted to a browser, which is > not nice for a packager who has the spec in front of them and need to go to > the website > 2. upstream lists http://github.com/zopefoundation/zope.hookable as their > website, not PyPI and I belive we should use the same URL as upstream Corrected both. I had gotten the impression that URLs full of variables were fine because "more variables" but I did find it annoying trying to then use those URLs to grab sources. > > Summary: Efficient creation of hookable objects > > The second summary could be DRY'ed by using: > > Summary: %{summary} Thanks > The big %if/%else between EPEL and Fedora could use a few empty lines around > the %else statement to make it clearer it's not part of the %install section. Fixed so it's more clear. > > -k "not test_pure_python" > > This could use an explanation in a comment. Removed that exclude because those tests actually pass. > > %{buildroot}/%{python3_sitearch}/zope/hookable > > This is unusual after %pytet --pyargs. Is it indeed needed? What about plain > zope.hookable here? plain zope.hookable works. Fixed. > And finally, my favorite: > > > %global pypi_name zope.hookable > > Is this worth having just for the 3 usages in the spec when everywhere else > we need to use zope-hookable or zope/hookable anyway? Personally I like the variables so I've opted to leave it here. I've seen them both ways and even seen the variable in specs slightly simpler than this. :shrug: As always, thank you for the feedback! https://src.fedoraproject.org/rpms/python-zope-hookable/blob/rawhide/f/python-zope-hookable.spec -- 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=2117112 _______________________________________________ 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
