https://bugzilla.redhat.com/show_bug.cgi?id=1859414 Vít Ondruch <vondruch@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+ --- Comment #9 from Vít Ondruch <vondruch@xxxxxxxxxx> --- (In reply to Pavel Valena from comment #7) > (In reply to Vít Ondruch from comment #3) > > - Also, while minor nit, there is probably missing '/' in the command for tests. > > Doesn't have to be there, and AFAIK it doesn't change the behaviour. I'll > add it for readability. I don't mind if you remove the `\` after tools, but I mind consistency. Thx > > * Keep the default generated BRs together: > > - It would be nice to keep these together: > > - Mixing rubygems-devel in between other requires for instance does not make the review/maintenance easier. > > Right, the intent was to have them sorted alphabetically, as the comment > says (I've used `sort -u`). Strange, how the `rubygems-devel` got mixed in between `rubygems()` then? Does the sort ignore brackets? Still, I would prefer to keep the default BRs on top, but that is my preference, I'll leave it up to you. > > * Circular dependency with rubygem-rails > > - I think that using `rails` gem on various places is shortcut which brings its own issues. I think it would be better to list all the dependencies excluding `rails` itself. > > Sure, let's trim those down. (Note that it's only a runtime dependency, so > it does't matter until something depends on actionmailbox at buildtime.) Not sure I understand what you mean by this? rubygem-rails depends on rubygem-actionmailbox and rubygem-actionmailbox had `BR: rubygem-rails`. That is IMO circular dependency which is good to break. So now it is much better. > > * Keep the original tests in place > > - I prefer to keep the expanded tests in place and copy them to the location they needs to be. But it might be confusing either way. > > Ok. Is there any benefit to having them duplicated? I am not saying there is benefit in duplication, but shuffling the directory on the disk does not help either. Of course I would prefer symlink if that was possible, but otherwise I think the `cp` has the closest behavior to `mv`. May be it could help if linked just the files, but that is OT for this review (unless you want to experiment a bit ;) ) ... > Thanks for the review! YAW. The package looks good => 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@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
