|Summary:||Review Request: python-mimerender - RESTful HTTP Content Negotiation for Flask, Bottle, etc.|
|Product:||[Fedora] Fedora||Reporter:||Adam Williamson <awilliam>|
|Component:||Package Review||Assignee:||Zbigniew Jędrzejewski-Szmek <zbyszek>|
|Status:||CLOSED RAWHIDE||QA Contact:||Fedora Extras Quality Assurance <extras-qa>|
|Fixed In Version:||Doc Type:||Bug Fix|
|Doc Text:||Story Points:||---|
|Last Closed:||2016-01-22 02:38:17 UTC||Type:||---|
|oVirt Team:||---||RHEL 7.3 requirements from Atomic Host:|
Description Adam Williamson 2016-01-20 21:48:28 UTC
Spec URL: https://www.happyassassin.net/reviews/python-mimerender/python-mimerender.spec SRPM URL: https://www.happyassassin.net/reviews/python-mimerender/python-mimerender-0.5.5-1.fc24.src.rpm Description: mimerender provides a decorator that wraps a HTTP request handler to select the correct render function for a given HTTP Accept header. It uses mimeparse to parse the accept string and select the best available representation. Supports Flask, Bottle, web.py and webapp2 out of the box, and it’s easy to add support for other frameworks. Fedora Account System Username: adamwill Really I just want to add this as it's a dep for recent versions of python-flask-restless, which is in the repos already but severely outdated. flask-restless added the dep in 0.13.0, current version is 0.17.0, we're stuck on 0.12.1.
Comment 1 Zbigniew Jędrzejewski-Szmek 2016-01-21 01:05:02 UTC
The usual: %sum is useless, just define Summary, then use %summary. + latest version + license is OK (MIT) + license file was requested ;) + name is OK + new python template is used + no scriptlets are needed or present + build requirements look sane - requirements seem incomplete: > # For some reason, RPM auto-provides doesn't catch this We have an autogenerator for python deps? I don't think so, all requirements must be specified manually. $ grep import /usr/lib/python3.5/site-packages/mimerender.py import mimeparse from functools import wraps import re import web import flask import bottle import webapp2 import unittest2 as unittest import unittest
Comment 2 Adam Williamson 2016-01-21 01:30:53 UTC
huh, I was sure there was a python dep generator. how is there not?! that be crazy. functools and re are stdlib stuff, unittest2 is only there so you can run the file directly to run the unit tests (as I do in %check) so it doesn't make sense to make it a requirement for normal use (normal use would be to import the module). The web, flask, bottle and webapp2 imports are all optional - basically for each one, if it's there, you get a MimeRenderBase subclass for it, if it's not there, you don't. I'm not sure how best to handle this, I don't think Requires or even Recommends make sense, because I figure the usual case is you're going to have a webapp using *exactly one* of those frameworks which wants to use mimerender, and the webapp itself will have the appropriate requirements anyway. If we add Requires or Recommends you're going to wind up with four webapp frameworks installed, likely three of which you don't need. I think the right thing to do is probably just to add a comment about this in the spec. WDYT?
Comment 3 Adam Williamson 2016-01-21 01:35:04 UTC
oh, there's also explicitly a use case for mimerender even if you have *none* of the webapp frameworks it provides an implementation for; you can still just create your own subclass of MimeRenderBase to integrate it with some other webapp framework. Which is another reason not to have any kind of dependency on the ones it provides an implementation for.
Comment 4 Zbigniew Jędrzejewski-Szmek 2016-01-21 02:40:11 UTC
Maybe Suggests? rpmlint: python2-mimerender.noarch: W: summary-ended-with-dot C RESTful HTTP Content Negotiation for Flask, Bottle, etc.
Comment 5 Adam Williamson 2016-01-21 04:38:47 UTC
I thought about Suggests, and it's the best candidate, but it still just doesn't feel right to me; no dependency still seems the best option. the dot is not terminating the sentence but representing the shortening of 'etcetera', so I think it's correct to keep it.
Comment 6 Zbigniew Jędrzejewski-Szmek 2016-01-21 13:42:27 UTC
Suggests is nice because it is a standard way to look for optional dependencies. I'd think this usecase is more or less it's purpose. Anyway, there is nothing wrong with the package. Package is APPROVED.
Comment 7 Adam Williamson 2016-01-21 16:37:58 UTC
Basically the reason it doesn't feel right to me is that I don't see where it'll be any *use*. So, let me try to articulate. For me the classic use of Suggests: is if a program works perfectly fine without something, but gets some extra capabilities if you add something. So far that sounds like this, right? But the thing is, the dependency is ultimately intended as a signal to a human; the idea is that package managers can tell humans 'hey, if you install this other package too, you'll get extra functionality!' That doesn't fit this case, for me. The reason being, I don't see any circumstance in which "install python-mimerender" is a sensible entry point to "get some web frameworks". I suspect the only use case for installing python-mimerender directly would be if you wanted to write some code using it, and I can't really see a scenario in which you're writing some kind of web service code and you start out by saying "OK, well, I DEFINITELY want to use python-mimerender. Now, what web framework should I use?" That just doesn't seem likely. Otherwise, python-mimerender is only ever going to be pulled in as a dep of something else which wants to use it, and I don't see that the Suggests: fulfils any useful function in this case, because as I said, whatever wants to use it will already have the necessary dependencies on web frameworks, and Suggesting other ones to a person who's just trying to install a webapp or something does not seem to be in their interest. So I just can't foresee a use case in which the Suggests actually improves anyone's experience, which is the ultimate goal. Do you? Thanks for the review! I'll change the Summary: bit on import.
Comment 8 Gwyn Ciesla 2016-01-21 20:21:09 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/python-mimerender
Comment 9 Adam Williamson 2016-01-22 02:38:17 UTC
Now built in Rawhide, and updates-testing pending for other releases. Thanks for the review.