Print scores of supporting leagues only

[carlosvargas]

The README specifies $ soccer --time=10 # scores for all the seven leagues over the past 10 days, however the code was pulling the scores from all of the leagues supported by the API. The update uses the league settings to fetch only the relevant fixtures.

[ueg1990]

Also given the structure of the code in the main function right now, maybe its better to push the if condition inside the get_scores function?

Maybe a default value should be passed to the league option (that could be set to None) and the logic for that default value could be handled in the get_scores_function

[ueg1990]

looks good to me :) but i still think the logic should be moved to inside the function get_scores. Lets see what others suggest

[carlosvargas]

@ueg1990 I thought about that putting it inside of get_scores, but wasn't sure about it.

I'm thinking of the following then:

def get_scores(league, time):
    def get_league_scores(league, time):
        league_id = LEAGUE_IDS[league]
        fixtures_results = requests.get('{base_url}soccerseasons/{id}/fixtures?timeFrame=p{time}'.format(
       ....

    if league is None:
        for league in LEAGUE_IDS:
            get_league_scores(league, time)
    else:
        get_league_scores(league, time)

If I move the current code inside a nested function, then I can still re-use the logic for multiple and single leagues.

What do you think?

[Saturn]

Isn't there a slight flaw in this? Instead of making one request, it will make one per league? Or am I misreading?

I think the code would need to filter out the "unsupported" league results rather than make individual requests.

The json data doesn't have a "League" attribute so you might have to look at one of the URL's to determine what league the fixture belongs to. For example:

http://api.football-data.org/alpha/soccerseasons/400 would correspond to the Spanish Segunda division. ( which is not supported :smile: )

leagueids.py would have to come into play.

LEAGUE_IDS = {
    "BL": 394,
    "FL": 396,
    "EPL": 398,
    "LLIGA": 399,
    "SA": 401,
    "PPL": 402,
    "DED": 404,
    "CL": 405
}

You could have a simple function that takes each individual fixture and returns True or False based on whether or not the URL ends with one of the codes above.

# Only print supported league results
for result in result_list:
   if is_league_supported(result):
        print result 

That is a better solution in my opinion. Thoughts?

Also, I 100% agree that this is a needed change. And I think the code would be better off inside of the get_scores function rather than within main()

[ueg1990]

@carlosvargas Your suggestion makes sense to me. Here is another suggestion:

if league is None:
    league_ids = LEAGUE_IDS
else:
    league_ids = [LEAGUE_IDS[league]]

for league in league_ids:
    .....
    .....

another advantage of this suggestion is that there is not need for any filtering. :)

[carlosvargas]

@Saturn that's right, it will make a request per each league.

The only thing about your suggestion, is that the response data will keep growing if api.football-data.org keeps adding more leagues, since it returns all of the games for that time frame. Not a big deal, but we most likely have to filter out quite a bit of the data (unless the user adds more leagues manually that is :smile: )

Also, by requesting them separately, it creates an easier grouping when printing the scores, since they will be automatically sorted by their league. This can still be done manually, but I thought this was a little easier.

@ueg1990 yep, that works. Thought about that after making my comment.

[Saturn]

@carlosvargas That is an interesting point. Although I doubt it will grow to be an unreasonable number to handle.

I think having fewer requests is better since it means it is nicer for the API server but also faster for the user since the filtering that would be done is practically instantaneous anyway.

I hadn't thought of the "league" sorting and that is obviously a vital thing. Would be really annoying to see a mishmash of different league fixtures like that.

It might be nice to have a League heading separating the fixtures too. I will be honest and say I had to Google some of the teams to understand what League they are in :smile:

[ueg1990]

lol i still do that :) it so gr8 to work on a python and open source project with fellow football (and for some ppl soccer) fans!!!

[carlosvargas]

OK that makes sense, I'll make it in one request and filter them out. I'll also group them manually then.

@ueg1990 totally agree and it's fútbol for some of us :smile:

[Saturn]

Petition to rename soccer-cli to fútbol-cli :yum:

[ueg1990]

lol....unicode issues :stuck_out_tongue: football-cli not-called-soccer-cli :smile:

[carlosvargas]

I have gone ahead and done this. Regarding the league header, I just left the league code in there for now. If we want the actual name, I can get it by requesting it from http://api.football-data.org/alpha/soccerseasons/401.

This is how it looks like after the grouping by league:

What do you guys think?

[ueg1990]

So in this one, this takes care of the case if you pass a league or not?

also, i still didnt get what is the purpose of the grouping that you did? just for curiosity :)

[ueg1990]

ok i see. when u do not pass a league, it gives all the results in no specific order (more likely by time). now your solution makes alot of sense :smile:

[carlosvargas]

Yes, this does 2 things:

1. If you don't pass a league, it will get all of the results for all of the leagues the API supports. Then it will filter the results out to only the leagues we are interested in.

2. It sorts the results by each league. By default, the API returns the list of scores sorted by the time, so you can have the scores of multiple leagues all jumbled up. For example:

   Chelsea vs ... Barcelona vs ... PSG... Bayern...

   Now imagine this for 70+ scores. This makes it difficult to see the scores of all EPL, La Liga, Bundesliga, etc games. All this does is just sort them by each league, so it's easier to quickly look at them.

[ueg1990]

The code looks good to me but i still think its better to filter first and then do a groupby.

Here is one suggestion:

supported_league_values = set(LEAGUE_IDS.values())
get_league_id = lambda x: int(x["_links"]["soccerseason"]["href"].split("/")[-1])
supported_league_fixtures = [fixture for fixture in total_data["fixtures"] 
if get_league_id(fixture) in supported_league_values]
supported_league_fixtures = sorted(supported_league_fixtures, key=get_league_id)
for league, scores in groupby(supported_league_fixtures, key=get_league_id):
    click.echo()
    click.secho("{:=^86}".format(supported_leagues[league]), fg="green")
    click.echo()
    for score in scores:
        yield score

[Saturn]

Nice work :+1:

I think the full league name would be better than the league id. I don't know what the best way to provide that would be. Yet another dict?

And there should be a little whitespace on either side of the league headings to make it a little easier to read.

@ueg1990 I suppose it does make more sense to filter out and then sort the data.

@carlosvargas I see you have also included the dates in the fixture list which I like. Although I wonder if it would be better to leave that to a different pull request since there is quite a lot going on with the sorting etc. Although if you think it goes hand in hand then that is cool too.

Most fixtures take place on a Saturday and Sunday. The print out might look a little less dense if it only printed the dates like this:

========================================EPL=========================================

2015-08-29  Newcastle United FC  0    vs    1 Arsenal FC    

            AFC Bournemouth      1    vs    1 Leicester City FC 

            Stoke City FC        0    vs    1 West Bromwich Albion FC   

            Manchester City FC   2    vs    0 Watford FC    

            Liverpool FC         0    vs    3 West Ham United FC    

            Aston Villa FC       2    vs    2 Sunderland AFC    

            Chelsea FC           1    vs    2 Crystal Palace FC 

            Tottenham Hotspur FC 0    vs    0 Everton FC    

2015-08-30  Southampton FC       3    vs    0 Norwich City FC   

            Swansea City FC      2    vs    1 Manchester United FC  

Although I don't know how good it would look when printed to the console. Definitely worth experimenting with though.

@carlosvargas Also is there a particular reason that the function starts with an underscore?

[carlosvargas]

@ueg1990 you're right. There's no need for the extra dictionary there. Will use the supported_leagues = LEAGUE_IDS.values() instead.

I omitted filtering first then grouping in order to avoid the extra loop, but I can do either way. Although I guess I can make it a generator expression that can get passed into the sorted. That would solve both issues.

 supported_league_fixtures = (fixture for fixture in total_data["fixtures"] 
                            if get_league_id(fixture) in supported_league_values)
 supported_league_fixtures = sorted(supported_league_fixtures, key=get_league_id)

@Saturn re full league names - yes, I'm trying to decide the best way of doing this. I wanted to write a memoized function that will get this from the server in order to avoid the extra dictionary. But Python2 doesn't have a memoized decorator, unlike Python3.

So I think the dictionary might be the only way. Now, should it be a dictionary just for get_scores or make it a module one like LEAGUE_IDS (just in case some other function wants the full name of the league as well)?

I can make the date a separate request, that's not a problem. We can discuss the display on that other task.

I added the _ out of habit when writing private functions. I guess it doesn't really make much sense here.

[Saturn]

@carlosvargas

So I think the dictionary might be the only way. Now, should it be a dictionary just for get_scores or make it a module one like LEAGUE_IDS (just in case some other function wants the full name of the league as well)?

It does kind of feel a bit messy having so many different dicts describing the league ids, codes etc. I don't really know what an elegant solution to that might be though.

I think it could be quite likely another function might want the full league names.

@ueg1990 @architv Any ideas about how to incorporate full league names?

Could a Named Tuple be used?

>>>EPL.code
398
>>>EPL.name
'English Premier League'

A solution that also helps solve the problem of the individual team codes and league properties would be a real improvement in my opinion. For example the named tuples could also include the properties defined in leagueproperties.py.

>>>EPL.relegation
[18,19,20]
>>>EPL.champ_league
[1,2,3,4]
>>>EPL.europa_league
[5]

I think it is not essential that this pull request solve the problem of the real league names though. The most important thing would be to get the sorting and filtering working nicely :+1:

[carlosvargas]

Updated the code with the following:

• The code now filters first, then groups leagues.
• Removed the _ in the function name.
• Removed printing the date.
• Added a space in the league code to make it easier to see.

I left the league code since fetching the full name is a bigger discussion.

[architv]

@carlosvargas Nice work! This makes a lot of sense. But shouldn't the date be included? I agree that most matches take place on the weekend, but we need to figure out a way to add date in a way that it doesn't look dense.

[architv]

@Saturn Yes, a named tuple for all the properties should be the best way to do it.

[Saturn]

Nice work :+1: