Bug 2368625

Copr build:
https://copr.fedorainfracloud.org/coprs/build/9081475
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2368625-python-pytest-notebook/fedora-rawhide-x86_64/09081475-python-pytest-notebook/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

- About test requirements, could you eliminate the `black`, `beautifulsoup4`, `coverage` tests and dependencies. Most likely these tests would not be run downstream either, and I am not sure how much you want to burden yourself with maintaining that part.
- About documentation, I believe the install location is not distro friendly? Could you move them to `%_pkgdocdir`, but also
- Do you really need/want to package the documentations. Especially for a project like this that would not have a user-facing audience
- The fact that `test_nb_regression` is failing, is quite telling. I think it would be cleaner to just ignore them altogether. Btw, nice bash-foo syntax on that
- I know upstream has quite a lot on his plate, but if you could coordinate and merge (as in squash) some of the open PRs, that would make it easier for him to catch up with this
- Could you add `README.md` to `%doc`? I am not aware of it being added automatically from `%pyproject_wheel` (though would be nice if it was)
- The "Remove coverage from tests" could you make them a patch instead? `sed` would not catch if the original source has changed, but the latter would.

(In reply to Cristian Le from comment #2)
> - About test requirements, could you eliminate the `black`,
> `beautifulsoup4`, `coverage` tests and dependencies. Most likely these tests
> would not be run downstream either, and I am not sure how much you want to
> burden yourself with maintaining that part.

Done

> - About documentation, I believe the install location is not distro
> friendly? Could you move them to `%_pkgdocdir`, but also

This is mostly for README, AUTHORS, Changelog and other similar files.

Documentation files for yelp go in %{_datadir}/help/ and then the
appropriate language folder.

Checking with Rüdiger Landmann who know about DocBook from maintaining
Publican https://src.fedoraproject.org/rpms/publican
Though may also need to ask on list.

> - Do you really need/want to package the documentations. Especially for a
> project like this that would not have a user-facing audience

Helpful.  Sometimes one is offline and wants documentation.

> - The fact that `test_nb_regression` is failing, is quite telling. I think
> it would be cleaner to just ignore them altogether. 

Helps verify functionality.


> Btw, nice bash-foo
> syntax on that

Not mine. Stolen from Ben Beasley.

> - I know upstream has quite a lot on his plate, but if you could coordinate
> and merge (as in squash) some of the open PRs, that would make it easier for
> him to catch up with this

Not a good fit for me. Rarely use Python notebooks.  If there are other things
in HPC/Materials Science that could be useful, happy to help with those. Can
also update pyproject.toml

> - Could you add `README.md` to `%doc`? I am not aware of it being added
> automatically from `%pyproject_wheel` (though would be nice if it was)

Done

> - The "Remove coverage from tests" could you make them a patch instead?
> `sed` would not catch if the original source has changed, but the latter
> would.

Done

spec: https://fed500.fedorapeople.org/python-pytest-notebook.spec
srpm: https://fed500.fedorapeople.org/python-pytest-notebook-0.10.0-1.fc41~bootstrap.src.rpm

Created attachment 2091720 [details]
The .spec file difference from Copr build 9081475 to 9092728

Copr build:
https://copr.fedorainfracloud.org/coprs/build/9092728
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2368625-python-pytest-notebook/fedora-rawhide-x86_64/09092728-python-pytest-notebook/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

(In reply to Benson Muite from comment #3)
> (In reply to Cristian Le from comment #2)
> > - Do you really need/want to package the documentations. Especially for a
> > project like this that would not have a user-facing audience
> 
> Helpful.  Sometimes one is offline and wants documentation.

Yes, but can it be accessed other than manually navigating to the index.html file? I've heard that KHelpCenter might do some magic, but I think it doesn't read `/usr/share/doc/<pkg>` format.

Anyway, at the very least let's avoid the bootstrap. You should be able to export a `PYTHONPATH` if the script requires it to be built first [1]

> > - The fact that `test_nb_regression` is failing, is quite telling. I think
> > it would be cleaner to just ignore them altogether. 
> 
> Helps verify functionality.

Up to you on this. Are there PRs that fix this test?

> > - I know upstream has quite a lot on his plate, but if you could coordinate
> > and merge (as in squash) some of the open PRs, that would make it easier for
> > him to catch up with this
> 
> Not a good fit for me. Rarely use Python notebooks.  If there are other
> things
> in HPC/Materials Science that could be useful, happy to help with those. Can
> also update pyproject.toml

Ok, if I remember, I will give it a shot.

[1]: https://sourcegraph.com/search?q=context:global+file:%5C.spec%24+PYTHONPATH&patternType=keyword&sm=0