Bug 2131705

Here's some drive by feedback:

There's new macros for shell completions, and all of the completions directories are owned by filesystem. You can apply this patch[1] to your specfile and see [2] for more information.

[1]: https://paste.sr.ht/~gotmax23/ad16ede10a059f561e61ce348dd38eb3abc7a22f
[2]: https://pagure.io/packaging-committee/issue/1202

You reference %{pypi_name}, but that doesn't seem to be defined anywhere in your specfile. Instead of defining that macro, I'd suggest replacing all instances of `%{pypi_name}` with the literal value.

```
# Remove bundled egg-info
rm -rf %{pypi_name}.egg-info
```

There's no dotref.egg-info file in the archive, and this wouldn't be necessary even if there was.


> Source:         %{url}/archive/refs/tags/v%{version}/dotref-%{version}.tar.gz

The preferred format is %{url}/archive/v%{version}/dotref-%{version}.tar.gz, but meh

---

It would be nice if you provided a link to a COPR or Koji scratch build.

Hi Max thanks for your remarks!

> There's new macros for shell completions, and all of the completions directories are owned by filesystem. You can apply this patch[1] to your specfile and see [2] for more information.

This is great information. I didn't know thank you.

> You reference %{pypi_name}, but that doesn't seem to be defined anywhere in your specfile. Instead of defining that macro, I'd suggest replacing all instances of `%{pypi_name}` with the literal value.

Ah yes. I originally tried this with pyp2rpm but realized some of the generation is with the old style macros.

> There's no dotref.egg-info file in the archive, and this wouldn't be necessary even if there was.

Good point.

> The preferred format is %{url}/archive/v%{version}/dotref-%{version}.tar.gz, but meh

I'm a fan of conventions +1

Copr Build: https://copr.fedorainfracloud.org/coprs/anthr76/dotref/build/4897386/
Spec URL: https://anthr76.fedorapeople.org/python-dotref/python-dotref.spec
SRPM URL: https://anthr76.fedorapeople.org/python-dotref/python-dotref-1.2.4-1.fc37.src.rpm

Howdy Anthony, I recognized your name from Twitter.  I can take this review.

Based on a quick skimming of the upstream README, this appears to be primarily an application, not a library.  Per the guidelines [0] it should be named dotref, not python-dotref.  Making that change will also have the pleasant side effect of no longer needing the python3-dotref subpackage, the %_description macro, or the `-n dotref-%{version}` flag for %autosetup.

What's the reason for using pytest to run the tests?  Upstream uses coverage [1], which we should avoid [2], but a simpler approach that avoids the additional build requirement would be to use `%{python3} -m unittest -v`.


[0] https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_application_naming
[1] https://github.com/ovk/dotref/blob/v1.2.4/Makefile#L5
[2] https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_linters

The %pytest macro runs the tests against the code we actually ship (i.e. what's installed in %{buildroot}%{python3_sitelib}) instead of the source tree, so it's preferable to use that as the test runner.

That's a fair point, but it can also be accomplished by setting an environment variable, which doesn't involve an unnecessary build requirement.

    PYTHONPATH=%{buildroot}%{python3_sitelib} %{python3} -m unittest -v

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.