[Bug 1869719] 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 Pavel Valena 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 -- 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 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1869719] 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 --- Comment #10 from Vít Ondruch --- (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. -- 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 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1869719] 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 Pavel Valena changed: What|Removed |Added Status|ASSIGNED|MODIFIED Fixed In Version||rubygem-image_processing-1. ||11.0-1.fc34 --- Comment #9 from Pavel Valena --- """ 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: SSS..S..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", (: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)? -- 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 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1869719] 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 --- Comment #8 from Vít Ondruch --- (In reply to Pavel Valena from comment #6) > > * `skip` vs `return` > > To me, the benefit of `skip` is that I see "S" 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? :) -- 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 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1869719] 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 --- Comment #7 from Gwyn Ciesla --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-image_processing -- 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 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1869719] 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
--- Comment #6 from Pavel Valena ---
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 "S" 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.
--
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
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
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@lists.fedoraproject.org
[Bug 1869719] 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 Jaroslav Prokop changed: What|Removed |Added Flags|fedora-review? |fedora-review+ --- Comment #5 from Jaroslav Prokop --- Yeah right,(In reply to Vít Ondruch from comment #4) > You have probably forgot fedora-review+? Right, package approved! -- 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 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1869719] 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 --- Comment #4 from Vít Ondruch --- (In reply to Jaroslav Prokop from comment #2) > Looks fine You have probably forgot fedora-review+? -- 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 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1869719] 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 Vít Ondruch changed: What|Removed |Added CC||vondr...@redhat.com --- Comment #3 from Vít Ondruch --- * 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`? -- 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 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
[Bug 1869719] 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
--- Comment #2 from Jaroslav Prokop ---
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`.
--
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
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
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@lists.fedoraproject.org
[Bug 1869719] 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 Jaroslav Prokop changed: What|Removed |Added Status|NEW |ASSIGNED CC||jar.pro...@volny.cz Assignee|nob...@fedoraproject.org|jar.pro...@volny.cz Flags||fedora-review? --- Comment #1 from Jaroslav Prokop --- I'll take this for a review. -- 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 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org
