[Bug 1869719] New: Review Request: rubygem-image_processing - High-level wrapper for processing images for the web with ImageMagick or libvips
https://bugzilla.redhat.com/show_bug.cgi?id=1869719
Bug ID: 1869719 Summary: Review Request: rubygem-image_processing - High-level wrapper for processing images for the web with ImageMagick or libvips Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: pvalena@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://download.copr.fedorainfracloud.org/results/pvalena/rubygems/fedora-r... SRPM URL: https://download.copr.fedorainfracloud.org/results/pvalena/rubygems/fedora-r... Description: High-level wrapper for processing images for the web with ImageMagick or libvips. Fedora Account System Username: pvalena
Builds & Tests run: https://gist.github.com/pvalena/fb84484168ee271f683a988b9aa636f1
https://bugzilla.redhat.com/show_bug.cgi?id=1869719
Jaroslav Prokop jar.prokop@volny.cz changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |jar.prokop@volny.cz Assignee|nobody@fedoraproject.org |jar.prokop@volny.cz Flags| |fedora-review?
--- Comment #1 from Jaroslav Prokop jar.prokop@volny.cz --- I'll take this for a review.
https://bugzilla.redhat.com/show_bug.cgi?id=1869719
--- Comment #2 from Jaroslav Prokop jar.prokop@volny.cz --- Looks fine, I just have 2 nits:
There is some documentation in `doc` directory in the image_processing project that I think should be included since it looks like it has more content than the generated docs. Maybe transform it into some kind of manpage or include it in the *-doc subpackage?
And just a triviality: This `%{gem_instdir}/image_processing.gemspec` could use a macro so it becomes `%{gem_instdir}/%{gem_name}.gemspec`.
https://bugzilla.redhat.com/show_bug.cgi?id=1869719
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |vondruch@redhat.com
--- Comment #3 from Vít Ondruch vondruch@redhat.com --- * Unversioned `%gemspec_remove` - I don't like the usage of `%gemspec_remove` without specifying versions. If I am not mistaken, when the dependency is removed upstream, then it would pass without noticing. It is not really big deal, but it is nice to know there is something to remove if it is not needed anymore.
* `skip` vs `return` - It probably won't make any real difference, but wouldn't it be better to use `sed -i '/Vips::/ i \ return' test/test_helper.rb` instead of `sed -i '/Vips::/ i \ skip' test/test_helper.rb`?
https://bugzilla.redhat.com/show_bug.cgi?id=1869719
--- Comment #4 from Vít Ondruch vondruch@redhat.com --- (In reply to Jaroslav Prokop from comment #2)
Looks fine
You have probably forgot fedora-review+?
https://bugzilla.redhat.com/show_bug.cgi?id=1869719
Jaroslav Prokop jar.prokop@volny.cz changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #5 from Jaroslav Prokop jar.prokop@volny.cz --- Yeah right,(In reply to Vít Ondruch from comment #4)
You have probably forgot fedora-review+?
Right, package approved!
https://bugzilla.redhat.com/show_bug.cgi?id=1869719
--- Comment #6 from Pavel Valena pvalena@redhat.com --- Thank you both for your reviews!
I've added %bcond_with to disable the tests once ruby-vips is in Fedora (the test suite suceeds without skips).
There is some documentation in `doc` directory in the image_processing project that I think should be included since it looks like it has more content than the generated docs.
Oh, you mean the upstream `doc` directory. Well, feel free to create a PR to add documentation into the gem (either generated, like other gems, or just the *.md files; that's up to the upstream).
This `%{gem_instdir}/image_processing.gemspec` could use a macro so it becomes `%{gem_instdir}/%{gem_name}.gemspec`.
Sure, can be both ways (updated).
_ _ _ _
- `skip` vs `return`
To me, the benefit of `skip` is that I see "SSSSSSSSS" in the test output, so I know the tests are skipped actually. `return` would just hide the issue, right?
- Unversioned `%gemspec_remove`
It fails actually: https://gist.github.com/pvalena/aeda48c6fa786f2f3d41f996c3a6c11a
An like I wrote previously, I'll not be running %gemspec_remove anyway, once ruby-vips is in Fedora.
https://bugzilla.redhat.com/show_bug.cgi?id=1869719
--- Comment #7 from Gwyn Ciesla gwync@protonmail.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-image_processing
https://bugzilla.redhat.com/show_bug.cgi?id=1869719
--- Comment #8 from Vít Ondruch vondruch@redhat.com --- (In reply to Pavel Valena from comment #6)
- `skip` vs `return`
To me, the benefit of `skip` is that I see "SSSSSSSSS" in the test output, so I know the tests are skipped actually. `return` would just hide the issue, right?
That is nice of course, but if the test case contains more asserts, the `skip` won't execute the following asserts, while the `return` does. So what are pros/cons? :)
https://bugzilla.redhat.com/show_bug.cgi?id=1869719
Pavel Valena pvalena@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED Fixed In Version| |rubygem-image_processing-1. | |11.0-1.fc34
--- Comment #9 from Pavel Valena pvalena@redhat.com --- """ the `skip` won't execute the following asserts """
Are you sure about that? I've always thought it's the other way around:
``` # SKIP: DEBUG: # Running: DEBUG: SSSSSSSSSSSSSSS..S.SSSSSSSS.....S..S...SSS..SSS...S.SS..S..S..................S.S...S.S. DEBUG: Finished in 3.638647s, 24.1848 runs/s, 20.3372 assertions/s. DEBUG: 88 runs, 74 assertions, 0 failures, 0 errors, 41 skips
# RETURN: DEBUG: + ruby -Ilib:test -e 'Dir.glob "./test/**/*_test.rb", &method(:require)' DEBUG: Run options: --seed 39129 DEBUG: # Running: DEBUG: ........................................................................................ DEBUG: Finished in 4.193257s, 20.9861 runs/s, 19.5552 assertions/s. DEBUG: 88 runs, 82 assertions, 0 failures, 0 errors, 0 skips ```
And with the same test-set, but vips enabled:
``` DEBUG: Run options: --seed 32767 DEBUG: # Running: DEBUG: ........................................................................................ DEBUG: Finished in 5.651056s, 15.5723 runs/s, 26.5437 assertions/s. DEBUG: 88 runs, 150 assertions, 0 failures, 0 errors, 0 skips ```
aaand ... now I'm even more confused. So the return has (8) more assertions, and `skip` does not count show all assertions skipped (return hides those, obviously)?
https://bugzilla.redhat.com/show_bug.cgi?id=1869719
--- Comment #10 from Vít Ondruch vondruch@redhat.com --- (In reply to Pavel Valena from comment #9)
""" the `skip` won't execute the following asserts """
The `skip` or `return` are placed into custom assertion if I am not mistaken. `skip` raises exception, so effectively fails the whole test case, while `return` just exits the specific assertion and let the following assert do its job.
https://bugzilla.redhat.com/show_bug.cgi?id=1869719
Pavel Valena pvalena@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |CLOSED Fixed In Version|rubygem-image_processing-1. |rubygem-image_processing-1. |11.0-1.fc34 |11.0-1.fc34 | |rubygem-image_processing-1. | |11.0-1.fc33 Resolution|--- |CURRENTRELEASE Last Closed| |2020-10-30 01:56:36
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com