-
-
Notifications
You must be signed in to change notification settings - Fork 189
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DEV: Added multiple countries view #104
base: master
Are you sure you want to change the base?
DEV: Added multiple countries view #104
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes requested
Since this PR meant to work on multiple countries, some things are not looking right:
- the API url is set to a single country;
- var name refers to a single country.
Could you please review these items ?
utils/getMultipleCountries.js
Outdated
module.exports = async (spinner, table, states, countryName, options) => { | ||
if (countryName && !states && !options.chart) { | ||
const [err, response] = await to( | ||
axios.get(`https://corona.lmao.ninja/v2/countries/${countryName}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this getting just 1 country ? Example:
https://corona.lmao.ninja/v2/countries/Brazil gets data from Brazil only. Shouldn't it stop on "/countries" ?
Click the link to see it: https://corona.lmao.ninja/v2/countries/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue #54 description states that we need to provide multiple countries to the command line input, for example in this case to test (since not yet deployed) the command :
node index.js --s country algeria,france,italy
Hence we are using the api which allows to get the specified countries input params :
https://corona.lmao.ninja/v2/countries/:countryList
{where :countryList -> (algeria,france,italy) in this case}
Thanks
utils/getMultipleCountries.js
Outdated
exitCountry(err, spinner, countryName); | ||
err && spinner.stopAndPersist(); | ||
handleError(`API is down, try again later.`, err, false); | ||
const thisCountry = response.data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's about multiple countries, maybe the name should have "countries"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree having countries as variable name seems more meaningful. I have changed that in the latest commit.
utils/getMultipleCountries.js
Outdated
// Format. | ||
const format = numberFormat(options.json); | ||
|
||
thisCountry.map(data => table.push([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in line 10, since https://corona.lmao.ninja/v2/countries/${countryName} refers to a single country, has this .map() worked ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a condition here, since previously it worked only for multiple countries input. Now it will work for single country, but the functionality here is for multiple country as mentioned above (#104 (comment))
utils/getMultipleCountries.js
Outdated
// Format. | ||
const format = numberFormat(options.json); | ||
|
||
thisCountry.map(data => table.push([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the output when I've tried to run this:
× UNHANDLED ERROR
× ERROR → TypeError
i REASON → thisCountry.map is not a function
i ERROR STACK ↓
TypeError: thisCountry.map is not a function
at module.exports (C:\Users\lukel\Desktop\development\corona-cli\utils\getMultipleCountries.js:20:15)
at processTicksAndRejections (internal/process/task_queues.js:97:5)
at async C:\Users\lukel\Desktop\development\corona-cli\index.js:67:2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed this issue in the latest commit, thanks for mentioning. Kindly test and update.
Thanks Luke for taking time to comment on the issue changes, have addressed your comments separately below each one of them. |
When it is run in minimal/ $ corona-cli portugal,spain -m
# Country Cases Cases (today) Deaths Deaths (today) Recovered Active Critical Per Million
→ Worldwide 42,847,157 383,248 1,153,216 4,503 31,602,038 10,091,903 76,918 5,497
—
# Country Cases Cases (today) Deaths Deaths (today) Recovered Active Critical Per Million
→ Worldwide 42,847,157 383,248 1,153,216 4,503 31,602,038 10,091,903 76,918 5,497
—
— Portugal 116,109 3,669 2,297 21 67,842 45,970 221 11,397
— Spain 1,110,372 0 34,752 0 0 1,075,620 2,031 23,746 |
Description:
Added option to view multiple countries data in table / chart format.
Instructions:
Multiple countries are input to the cli as below format in order for the data retreival of multiple countries.
Run command:
corona --s country india,ecuador,portugal
Fixes #54