microCOVID icon indicating copy to clipboard operation
microCOVID copied to clipboard

Make population a number instead of a string

Open GayHackRat opened this issue 4 years ago • 6 comments

Made population an integer in update_prevalance.py:AppLocation, ran yarn prevelance and updated frontend code accordingly.

GayHackRat avatar Sep 13 '21 21:09 GayHackRat

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

GayHackRat avatar Sep 13 '21 21:09 GayHackRat

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Sep 13 '21 21:09 netlify[bot]

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 avatar Sep 13 '21 22:09 blanchardjeremy

@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

GayHackRat avatar Sep 14 '21 01:09 GayHackRat

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 avatar Sep 14 '21 14:09 blanchardjeremy

@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

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.

beshaya avatar Sep 17 '21 19:09 beshaya

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.

jaraco avatar Mar 25 '23 02:03 jaraco

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.

jaraco avatar Mar 25 '23 02:03 jaraco