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.

Sign up for GitHub

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

Jump to bottom

DEV: Added multiple countries view #104

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

DEV: Added multiple countries view #104

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9796cbd
DEV: Added multiple countries view
Shivathanu May 10, 2020
f07a646
FIX: Address PR comments
Shivathanu May 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const theEnd = require('./utils/theEnd.js');
const handleError = require('cli-handle-error');
const getStates = require('./utils/getStates.js');
const getCountry = require('./utils/getCountry.js');
const getMultipleCountries = require('./utils/getMultipleCountries.js');
const getCountryChart = require('./utils/getCountryChart.js');
const getBar = require('./utils/getBar.js');
const getWorldwide = require('./utils/getWorldwide.js');
Expand Down Expand Up @@ -63,6 +64,7 @@ const options = { sortBy, limit, reverse, minimal, chart, log, json, bar };
spinner.start();
const lastUpdated = await getWorldwide(output, states, json);
await getCountry(spinner, output, states, country, options);
await getMultipleCountries(spinner, output, states, country, options);
await getStates(spinner, output, states, options);
await getCountries(spinner, output, states, country, options);
await getCountryChart(spinner, country, options);
Expand Down
35 changes: 35 additions & 0 deletions utils/getMultipleCountries.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
const axios = require('axios');
const numberFormat = require('./numberFormat');
const exitCountry = require('./exitCountry');
const to = require('await-to-js').default;
const handleError = require('cli-handle-error');

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

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.

`—`,
data.country,
format(data.cases),
format(data.todayCases),
format(data.deaths),
format(data.todayDeaths),
format(data.recovered),
format(data.active),
format(data.critical),
format(data.casesPerOneMillion)
]));
spinner.stopAndPersist();
console.log(table.toString());
}
};