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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEV: Added multiple countries view #104

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

Conversation

Shivathanu
Copy link

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

Copy link

@ghost ghost left a 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 ?

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}`)
Copy link

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/

Copy link
Author

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

exitCountry(err, spinner, countryName);
err && spinner.stopAndPersist();
handleError(`API is down, try again later.`, err, false);
const thisCountry = response.data;
Copy link

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"

Copy link
Author

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.

// Format.
const format = numberFormat(options.json);

thisCountry.map(data => table.push([
Copy link

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 ?

Copy link
Author

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

// Format.
const format = numberFormat(options.json);

thisCountry.map(data => table.push([
Copy link

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

Copy link
Author

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.

@Shivathanu
Copy link
Author

Thanks Luke for taking time to comment on the issue changes, have addressed your comments separately below each one of them.

@rsubtil
Copy link

rsubtil commented Oct 24, 2020

When it is run in minimal/-m mode, it shows duplicated content:

$ 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

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

Successfully merging this pull request may close these issues.

add multiples countries
2 participants