https://bugzilla.redhat.com/show_bug.cgi?id=2140295 Ankur Sinha (FranciscoD) <sanjay.ankur@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED --- Comment #3 from Ankur Sinha (FranciscoD) <sanjay.ankur@xxxxxxxxx> --- Thanks very much for the review fedepell :) (In reply to fedepell from comment #2) > Hello! > Looks mostly all fine! > > Minor comments: > > -) Dependencie just wonderign: you added sympy which is an optional, but not > pyarrow which is the other optional, is there a reason for this? > > > BuildRequires: python3-sympy They should ideally all be pulled in automatically. sympy was required for one of the tests and wasn't pulled in, so I've added it manually there. pyarrow was not required for the tests from the looks of it. > > Just another side note on dependencies: due to > "python3dist(typing-extensions) >= 4.2" this will only build with FC37 or > higher. And of course depending if you release interface-meta (and its > dependencies) in FC37 in the future :-) I intend to push the deps to all releases---they're in side tags at the moment, but yes, if the necessary version of typing-extensions isn't in Fedora=<37, we won't be able to build this package there. I can ask the maintainers if they'd consider updating, but going from 3.x to 4.x is a major update, so they may not want to do that in Fedora<=37 :( > > -) Add license file explicitly that is missing? So add under %files: > > > %license LICENSE In this one, the pyproject macros mark the LICENSE file already, so we don't need to do it again: rpm -ql --licensefiles -p ./results_python-formulaic/0.5.2/3.fc38/python3-formulaic-0.5.2-3.fc38.noarch.rpm /usr/lib/python3.11/site-packages/formulaic-0.5.2.dist-info/licenses/LICENSE > -) I read the comments on the disabled tests and the upstream thread. I saw > they were just recently fixed here: > https://github.com/matthewwardrop/formulaic/commit/ > e5dedcb0feed39f5ff6e2326d727ca65d247f26d > Would it be worth to add this patch (to be removed at next package update) > so then we can run all the tests probably? Yeh, I've backported that now, and all tests are enabled. Updated spec/srpm: https://ankursinha.fedorapeople.org/python-formulaic/python-formulaic.spec https://ankursinha.fedorapeople.org/python-formulaic/python-formulaic-0.5.2-3.fc38.src.rpm Thanks again, Ankur -- 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 https://bugzilla.redhat.com/show_bug.cgi?id=2140295 _______________________________________________ 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 Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
