Refactor writers

[ueg1990]

I have made a few changes in the writers base class. Let me know what you think.

I think i would like your opinion on is too divide the writers into different modules like the following structure: soccer/ soccer/ writers/ writer.py -> class BaseWriter would move here stdout_writer.py csv_writer.py json_writer.py

If we do this then the function get_writer (from writers.py) can be moved to main.py and it will be something like this in main.py:

from writers imprt Stdout, Csv Json
....
....
def get_writer(output):
    return globals()[output.capitalize()]()

[ueg1990]

@carlosvargas just to let you know, the reason i changed Console class name with Stdout so that its easier to find among the globals() function

Other points: 1) I moved supported_leagues function as part of the BaseWriter because its common in Csv and Json. But the supported_leagues method is modified for Stdout because it prints league names as part of the print 2) Method league_header is only used in Stdout and its purpose is printing to stdout so i made a method of only Stdout

[carlosvargas]

@ueg1990 those changes look to me. The only caveat is that we now have duplicated code, just to have the header printed. Maybe we can just add a flag to the Base class method:

def supported_leagues(self, total_data, should_write_header=false):
   ...
   if should_write_header:
       self.league_header(league)
   ....

Also, it seems to me that commit 276a70e Add steps to add api token as an env variable could be a separate PR.

[ueg1990]

i agree with the flag...i initially had that before too. Will add the flag and push another commit for review. the steps parts is just to update the README. didnt want to do another PR just for that.

[ueg1990]

I have send another a commit for review.

Also, what do you guys think of moving each writer class to its own module?

[carlosvargas]

@ueg1990 I don't think we need to split each of these classes into each own module. I don't really see a need for it. Is there a specific reason that I'm not seeing?

[ueg1990]

the splitting of the classes was just a suggestion as it can keep our code cleaner and files will not be too big.

[carlosvargas]

Looking at flask and requests they have a couple of pretty big files with multiple classes in it ( request/models.py), so that might not be a bad idea to have.

If we split it into separate modules, maybe we can put them in a writerssubpackage though, as to clearly separate them from the rest of the files. But Flat is better than nested. so maybe not?

What do @Saturn and @architv think?

[Saturn]

I am a little behind on any new developments.

I see writers.py could become a little long but splitting it up could possibly convolute what is already there.

Also as I have said before I'd really like to be able to use it in a manner like this:

soccer --standings --league EPL --json -o premier_league_standings.json

or

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

Without the -o flag it just prints to stdout.

Also any thoughts on 'prettying' the output of the json? Like:

json.dump({'standings': data}, json_file, indent=4, separators=(',', ': '))

Just to make it more pleasant to read.

:+1:

[ueg1990]

For the following type of command:

soccer --standings --league EPL --json -o premier_league_standings.json

it may make the parser more complicated because u will have to check for -o if --json is passed.

What about if we do it without the -o; as in pass the name of the file to --json. In that way we will have --json , --csv and if none of them are provided, we print to stdout

another concern i have if we add optional arguments, then we will have more arguments in the main function....which might also make it convoluted.

If you guys want to try to --json , --csv , then i can send another commit in the pull request

[Saturn]

I thought it might have been simpler if --json & --csv were simple boolean flags rather than choices via --output. You can't have both in the same call of course. And if none are present then the user just wants the nice readable output.

And I imagined when --output is not called it would just print to the screen as normal. But when it is present, it means the user wants to save the output in a file instead of printing.

I think that is the most elegant solution in terms of the interface for the user. At least that is what feels natural to me when using a cli to do similar tasks.

# figuring out what the user wants
if json and csv:
    # user done something wrong :(
if json:
    # user wants json output
if csv:
    # user wants csv output
if output:
   # user wants output saved in file named in variable output
# etc etc etc 

[ueg1990]

Update: 1) @Saturn for your suggestion, i found something useful: http://click.pocoo.org/5/options/#feature-switches This ensures that --csv, --json and default (stdout) are mutually exclusive Also your suggestion for formatting json output was beautiful!!! :smile: 2) @carlosvargas what do u think of the changes for printing league headers? I removed the method league header and put is as part of the league_scores method under Stdout class

[carlosvargas]

@ueg1990 that works much better than what I had suggested. Nice job :+1:

[Saturn]

i found something useful: http://click.pocoo.org/5/options/#feature-switches This ensures that --csv, --json and default (stdout) are mutually exclusive

That is beautiful. Click seems really nice. I will have to make something cool using it one of these days.

:+1:

[Saturn]

I am getting an import error with exceptions. I assume it works once it is installed via setup.py.

Is there any way to make it work when running python soccer/main.py --league LLIGA.

Without having to rename exceptions.py.

Does this work ?

from soccer.exceptions import IncorrectParametersException

Should all of our 'relative' imports be explicit like that to avoid conflicts?

[Saturn]

I think this error message

Printing output to stdout and saving to a file are mutually exclusive

Could possibly be more helpful if it said there needs to be a --json or --csv flag.

Of course it is quite easy to save the stdout to file via soccer --league EPL --standings > epl_table.txt

Of course no pretty colours this way :smile:

[architv]

@ueg1990 I've been a little behind all the new work that's been done. I checked out the changes but when I try to run it, I get

File "soccer/main.py", line 11, in <module>
    from exceptions import IncorrectParametersException
ImportError: cannot import name IncorrectParametersException

But if I change from exceptions import IncorrectParametersException to import exceptions and then change the exception call to exceptions.IncorrectParametersException('<message>') it works. Why is that so?

[architv]

@ueg1990 Other than that code looks really well structured now. Great job!

[Saturn]

I think it should be changed to

from soccer.exceptions import something

This way it won't import the standard exception module no matter how you run the script.

[architv]

Ah! Yes, I'll do that.

[ueg1990]

agreed with @Saturn but im still curious to know why it didnt fail for me. but ur way of importing isalos the preferred way to import in Python :)