Add utilities for message schemas #108
Conversation
Codecov Report
@@ Coverage Diff @@
## master #108 +/- ##
==========================================
+ Coverage 96.99% 97.58% +0.59%
==========================================
Files 12 13 +1
Lines 1064 1077 +13
Branches 147 149 +2
==========================================
+ Hits 1032 1051 +19
+ Misses 22 17 -5
+ Partials 10 9 -1
Continue to review full report at Codecov.
|
fedora_messaging/schema_utils.py
Outdated
|
|
||
| # https://github.com/fedora-infra/fedmsg_meta_fedora_infrastructure/issues/320 | ||
| HARDCODED_AVATARS = { | ||
| "bodhi": "https://apps.fedoraproject.org/img/icons/bodhi-{size}.png", |
There was a problem hiding this comment.
Hmm. I think this should live in the base class of the application's message schema (or generally in their repository). I would not expect to look here when the hardcoded URL eventually breaks. Nor would I want to accept pull requests from applications adding their URLs here. What's your motivation for including it here rather than in the schema packages?
There was a problem hiding this comment.
Nah you're right, I think I just stared at fedmsg code for too long, that's all.
| return libravatar.libravatar_url( | ||
| email=email, openid=openid, size=size, default=default | ||
| ) | ||
| params = collections.OrderedDict([("s", size), ("d", default)]) |
There was a problem hiding this comment.
urlencode accepts plain dictionaries, why does this have to be an ordered dictionary?
There was a problem hiding this comment.
I think it's only to make testing easier.
There was a problem hiding this comment.
It might be worth adding a comment to that effect to save confusion later.
abompard
force-pushed
the
schema-utils
branch
2 times, most recently
from
8725018
to
a0814b8
Compare
| return libravatar.libravatar_url( | ||
| email=email, openid=openid, size=size, default=default | ||
| ) | ||
| params = collections.OrderedDict([("s", size), ("d", default)]) |
There was a problem hiding this comment.
It might be worth adding a comment to that effect to save confusion later.
fedora_messaging/schema_utils.py
Outdated
| from six.moves.urllib import parse | ||
|
|
||
| try: | ||
| import libravatar |
There was a problem hiding this comment.
When these APIs are added to the documentation (do you want to do that here or in a later PR?) it should probably note this optional dependency.
abompard
force-pushed
the
schema-utils
branch
from
a0814b8
to
c0df7ce
Compare
|
Good point, I've added the docs now :-) |
At the moment it's mostly functions to generate avatar URLs. Signed-off-by: Aurélien Bompard <aurelien@bompard.org>
abompard
force-pushed
the
schema-utils
branch
from
c0df7ce
to
4491ff4
Compare
✨
At the moment it's mostly functions to generate avatar URLs.