architv / soccer-cli

:soccer: Football scores for hackers. :computer: A command line interface for all the football scores.
MIT License
1.09k stars 222 forks source link

Allow to store results to csv/json (Work In Progress) #36

Closed ueg1990 closed 8 years ago

ueg1990 commented 8 years ago

Right now I defined it only for live scores. If you have any feedback let me know; till then I will define it for the other functions.

@carlosvargas hopefully you can integrate your latest pull request after that :smile: Thank you for your patience

Saturn commented 8 years ago

The outputs do work for me.

I wonder if it would be an idea to allow an output filename as an argument. Which might add flexibility.

soccer --league EPL --standings --json --output file.json

Or just to be able to nicely redirect:

soccer --league EPL --standings --json | less

This pull request shows why we would need to separate out some functionality into separate modules to make it easier to manage.

I have made a pull request https://github.com/architv/soccer-cli/pull/38 to move things into a package if you want to take a look. I am guessing it needs to be done at some point so why not now?

ueg1990 commented 8 years ago

I was initially allowing users to pass a name to their file but it was adding unneeded complexity because of the case for print to stdout. i can add that as a separate pull request.

And i agree about the separation of modules and was also going to suggest that. ill take a look.

Saturn commented 8 years ago

I was initially allowing users to pass a name to their file but it was adding unneeded complexity because of the case for print to stdout. i can add that as a separate pull request.

That is understandable.

I was wondering how the stdout functions were any different from what it is normally but now I see you have just replaced the old print_somethings.

I kind of like the idea of being able to print json to stdout. Kind of like how the standard output is for human readability and json for machine readability.

Is doing soccer --league EPL --standings --output stdout any different than not defining an output at all?

ueg1990 commented 8 years ago

By default, it will print to stdout. So if you just do: soccer --league EPL --standings

This is going to print to stdout. Its so that if users want to output to csv and json, they can then pass the relevant parameters by passing '-o csv' or '-o json'

architv commented 8 years ago

@ueg1990 The ouputs to csv and json work well. We could allow users to provide a name to the files. In case of -o stdout we could just ignore the file name.

ueg1990 commented 8 years ago

The user does not have to pass '-o stdout'. I will create another pull request in which: 1) I will allow user to provide name for a file 2) If user does not provide name of output file, i will use the file names that i defined in this pull request 3) If user does provide a filename with '-o stdout', i will print an error message letting them know that they should not use the file option with the '-o stdout' option Let me know what you think :)

Saturn commented 8 years ago

@ueg1990 That sounds good.

architv commented 8 years ago

@ueg1990 +1

carlosvargas commented 8 years ago

@ueg1990 +1