Make population a number instead of a string
Made population an integer in update_prevalance.py:AppLocation, ran yarn prevelance and updated frontend code accordingly.
How do we generally handle these backward-incompatible changes in update_prevelance.py? I just ran it on the branch here but I can imagine supporting both at first until it is run or something
Deploy Preview for microcov ready!
| Name | Link |
|---|---|
| Latest commit | 478ec12831364fffa3189bfaf46fe06577a5d71d |
| Latest deploy log | https://app.netlify.com/sites/microcov/deploys/641e5b8c9b3b20000846146a |
| Deploy Preview | https://deploy-preview-1121--microcov.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
How do we generally handle these backward-incompatible changes in
update_prevelance.py? I just ran it on the branch here but I can imagine supporting both at first until it is run or something
Not sure what you mean. Why would we need to support both?
I'm not sure if this is what you're asking, but on a related note: we usually don't run update_prevalence on the same branch that does the changes. One branch does the changes. Then right after we merge it into main, we run the script and update all the data.
That said, I think what you're pointing out is that a bunch of stuff will break if we do it as two separate changes. So doing it exactly the way you have makes sense to me! 👍
Still not clear what "support both" would mean in this case, though.
@blanchardjeremy By "support both" I mean write frontend code that supports either format in the location.ts file (which would involve casework: parse it to a number if it's a string, otherwise don't), so that it works both before and after we run update_prevalence.py. Then once we run update_prevalence.py we could remove the casework in a later PR. That's generally how breaking backend changes are handled
Gotcha. Yeah. In that case , I think it'd be easier to just merge in the or the way you have it, where we have already run update prevalence as part of it.
@blanchardjeremy By "support both" I mean write frontend code that supports either format in the
location.tsfile (which would involve casework: parse it to a number if it's a string, otherwise don't), so that it works both before and after we runupdate_prevalence.py. Then once we runupdate_prevalence.pywe could remove the casework in a later PR. That's generally how breaking backend changes are handled
Yeah, that's what I'd suggest if we had a real backend, but since it's secretly all frontend and changeable atomically, this looks like the right approach.
I found the "population as string" surprising while working on some other changes recently. I'd like to see this merged, especially since the prevalence script concerns are no longer relevant.
Well, I attempted to merge main and resolve conflicts, but it does appear as if there's going to be quite a bit more work to do as the code is still complaining about str/int conflicts, both in javascript and Python, so someone will need to invest more time in getting things working again.
I'm going to close this PR for now, but I welcome others to revive it or use it as a guide to re-implement it. I may do the same too, but for now it's too much for me to take on.