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

Add ability to output fetched data in JSON and CSV formats. #43

Merged
merged 18 commits into from
Jul 14, 2018

Conversation

armansujoyan
Copy link
Contributor

Description of new feature, or changes

Implemented the ability to output fetched data in JSON and CSV formats by
giving --json and --csv flags, followed by file names. If there are no file names
specified in the CLI, by default files are saved as footballOut.csv or footballOut.json.

Related Issues and Discussions

Feature Request: Allow exporting output as JSON or CSV #20

People to notify

@manrajgrover

New --json and --csv  options for scores,fixtures,standings.
New feature for exporting fetched data as a JSON file.
Fix issues with export function, set default name for output.
Add error handling for the exporting function.
Aletered output function to support CSV.
@manrajgrover manrajgrover self-requested a review June 27, 2018 16:22
@manrajgrover
Copy link
Owner

Hi @armansujoyan,

I had a quick look and overall it looks great! Could you also add a flag for providing an output directory for json/csv? We can default to current directory if flag is not set.

@armansujoyan
Copy link
Contributor Author

I don't think there is need to add that flag. Righ now instead of the name you can write something like folder/filename and it will output the file there. Example of call is football fixtures -l WC --json stats/fixturesWC. This will output the file to the folder stats with the name fixtures.json, if the folder stats is available.

@armansujoyan
Copy link
Contributor Author

Done!

@armansujoyan
Copy link
Contributor Author

@manrajgrover I have added another flag for showing the directory where the data needs to be saved.

@manrajgrover
Copy link
Owner

@armansujoyan Thank you for working on this. I'll review this ASAP.

Copy link
Owner

@manrajgrover manrajgrover left a comment

Choose a reason for hiding this comment

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

Hi @armansujoyan,

Could you also document the feature?

Copy link
Owner

@manrajgrover manrajgrover left a comment

Choose a reason for hiding this comment

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

Hi @armansujoyan, the output requires improvements which I plan to do after merging this.

Thanks for working on this! LGTM! 😄

@manrajgrover manrajgrover merged commit e2c8efb into manrajgrover:master Jul 14, 2018
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.

2 participants