DEV: Added multiple countries view#104
Conversation
ghost
left a comment
There was a problem hiding this comment.
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.
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.
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 :
{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.
Since it's about multiple countries, maybe the name should have "countries"
There was a problem hiding this comment.
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.
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.
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.
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.
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,portugalFixes #54