Add an app to update the email signature PNG daily. #122
Conversation
|
I'll get to a proper review on this later today or tomorrow! Thanks for coming back to this :) EDIT: CI seems to be complaining about missing a |
carbondoomsday/settings.py
Outdated
| 'carbondoomsday.email_signature.tasks.' | ||
| 'update_email_signature' | ||
| ), | ||
| 'schedule': timedelta(days=1) |
There was a problem hiding this comment.
Perhaps this should be fired manually from the other celery tasks. For example, if the latest data updates at some strange time and this runs before that, the signature will not have the latest data. I think you'll want to try with apply_async.
| # https://stackoverflow.com/a/273962/2532070 | ||
| # https://stackoverflow.com/a/2563883/2532070 | ||
| # add_border_radius method: | ||
| # https://stackoverflow.com/a/11291419/2532070 |
There was a problem hiding this comment.
If these are useful to help understand the following code then I'd rather see them closer to the code that is using something from them. That might be in a function docstring in the usual """ my docstring """? If more general, could also use the module docstring at the top here.
|
Looks good! Nice one! Although I do question whether we need to the separate app for it? I imagined it being included in the original carbondoomsday app. I'm going to submit a PR (see #123) to clear up our current CI pipenv issues. That might give you some conflict issues which you'll need to rebase over and resolve but you'll get a clean CI run then! |
|
@lwm do you mean to day the "measurements" app? I didn't think the signature was really a measurement so put it in a new app. Happy to move it to the measurements app if you prefer. I usually try to make apps do one thing only and most of my apps are quite simple, but I really don't know if that's a good practice or not! |
YPCrumble
force-pushed
the
add_email_signature
branch
from
89a074f
to
c04d7b8
Compare
I approve of your zealous UNIX attitude! Let's do it. |
|
The CI failures probably needs a similar to #121 (comment) treatment? |
| from carbondoomsday.measurements.models import CO2 | ||
|
|
||
|
|
||
| # ppm = "412.32" |
|
OK, let's get this PR going green and I will try to do some background work to get you able to make deployments and such. Then we deploy and therefore win. |
YPCrumble
force-pushed
the
add_email_signature
branch
2 times, most recently
from
a0093e1
to
49fecf3
Compare
|
@lwm I think I'm getting closer - the isort functionality is still confusing me. This is the result from TravisCI: https://cl.ly/0e0y3K3I142M This is the result when I run |
|
@lwm nevermind - this is related to the fact that |
Codecov Report
@@ Coverage Diff @@
## master #122 +/- ##
==========================================
+ Coverage 97% 97.61% +0.61%
==========================================
Files 25 27 +2
Lines 534 546 +12
==========================================
+ Hits 518 533 +15
+ Misses 16 13 -3
Continue to review full report at Codecov.
|
|
@lwm I'm getting closer! You run a tight ship here. Codecov is complaining because my settings.py file isn't fully tested. My suggestion is we either reduce the coverage % or exclude settings.py from coverage - does one or the other (or something else) work for you? |
|
Nice one!
😆 TBH, I'm easy with whatever you can configure. The changes looks good to me, from another brief look. Can you send me your SSH public key part? I'll get you access to the machines. |
YPCrumble
force-pushed
the
add_email_signature
branch
2 times, most recently
from
7c6e9c7
to
04b66df
Compare
Remove unnecessary comment. Use isort to update import order. Add PIL to known_third_party settings. Update pipfile.lock.
YPCrumble
force-pushed
the
add_email_signature
branch
from
deb02b9
to
55b19c0
Compare
|
Ahhhhh I'm green! If this now looks good, would you pull in and I can:
What's your email to send the public key to @lwm ? |
|
Send it over to lukewm AT riseup.net (here's my key if you're into that stuff). I'm going to merge this because I want to get off Github tonight (if possible). I'll add an issue on the migrated repository with those 1. and 2. points! Thanks for this work! Solid stuff 🔥 🔥 🔥 |
|
🚀 |
@lwm in the spirit of moving quickly, this is a preliminary PR for adding the email signature png.
The path should be:
I wrote a couple tests for the Python portion of the implementation, but I'm less confident about my celery and dokku integrations...and not sure how to build out the staging server so they're untested. Would love any feedback and happy to make adjustments as required. Thanks!