Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃憣 Add Recovery Rate metric #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

josephkerkhof
Copy link

This PR resolves the request in #4. I changed the wording from mortality rate to recovery rate out of my own personal preference.

I'm not a JS developer by trade, so please let me know if you have any feedback. This looks like a cool project.

Comment on lines 12 to 19
/**
* Calculates the recovery rate from an array of data and splices the recovery rate
* into the middle of the array for table formatting.
*
* @param {Array} one data about the element we are processing. (e.g. state, country, etc.)
* @return {Array} A spliced array almost as it was before but now with a string formatted recovery rate.
*/
function spliceRecoveryRate(one) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one is a poor name, as a reader I have no idea what that means.
Perhaps area would be better?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one is the naming convention already in use in this project elsewhere for a single entry.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can make one better everywhere.

utils/calcRecoveryRate.js Outdated Show resolved Hide resolved
utils/calcRecoveryRate.js Outdated Show resolved Hide resolved
utils/calcRecoveryRate.js Outdated Show resolved Hide resolved
@ahmadawais
Copy link
Owner

Thanks a lot @musicaljoeker for sending in a PR. Good stuff. And thank you @Inukares for reviewing it. I'll check it out tomorrow. We'll soon have this here.

@ahmadawais
Copy link
Owner

Thanks for the PR. Can you please update the PR in light of the latest release.

馃敆 https://github.com/ahmadawais/corona-cli/releases/tag/3.4.0
馃敆 https://www.npmjs.com/package/corona-cli

@josephkerkhof
Copy link
Author

Hi @ahmadawais,

Sorry for the later response - I was finishing up my grad midterm project this week. I'll be happy to do that. I'll see if I can get to it tonight, but if not, I'll do it tomorrow.

@josephkerkhof
Copy link
Author

Ok I brought in the latest changes. Should be good to go hopefully 馃

@kushagra-shukla
Copy link

Hi @ahmadawais,
As this issue is still open I assume help is needed. I think I can do this, in case original author of this PR isn't working to resolve conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants