baronet2 / FirstCyclingAPI

An unofficial Python API wrapper for firstcycling.com
https://firstcyclingapi.readthedocs.io/en/latest/
MIT License
25 stars 3 forks source link

Create a distinction between old style race and new style race 3rd try #30

Open psemdel opened 1 year ago

psemdel commented 1 year ago

Sorry, it fell from my table ;).

Let's try again :)² I pulled from your last changes, so there should not be conflict anymore ;).

I added more tests. Some are noted with "### Following tests work after this PR" some with "###Following tests don't work with this PR, more code is required". You can try with your present code to be convinced.

Target of this pull request is to be able to read the classifications for "new style race" like Itzulia Women. To avoid making too much changes at a time, this PR is only the first step. It populates standings. An object Standing() is created, to be able to access the results using obj.results_table. Without this object the behavior would be different for "old style race" and "new style race" (one would be accessed with res.standings["gc"] and the other with res.standings["gc".results_table --> bad).

Part2 of the PR will be to resolve the

r=r_2023.results(classification_num=2).results_table assert len(r) == 18 assert r['Rider'].iloc[0] == 'Wyllie Ella' assert r['Time'].iloc[0] == "10:04:05"

Presently, the API will call https://firstcycling.com/race.php?r=14244&y=2023&e=&l=2 But as the parameter "l" is not supported it redirect to https://firstcycling.com/race.php?r=14244&y=2023&e= and we get the gc instead of the youth classification.

This will be solved by the use of standings.

baronet2 commented 1 year ago

Thanks @psemdel , really appreciate your help here. 😄

To make sure I understand, is this PR only to address the different syntax between lines 3 and 4 below?

from first_cycling_api import Race
basque = Race(6)
basque.edition(year = 2023).results(classification_num = 2).standings['youth'] # new
basque.edition(year = 2022).results(classification_num = 2).results_table # old

Is there any other problem that the Standings class will solve?

If not, I personally think the current behaviour is acceptable and am not sure adding the Standings class is worth the added complexity. What do you think?

psemdel commented 1 year ago

Hi, the target of this Standings is to be able to handle other classifications for "new" races.

Without the class Standings, you would have to make a try/except everywhere, as sometimes your result would be in "result_tables" (for old races) and sometimes directly at the root (for new ones).

Final result of my modifications, can be understood in the tests https://github.com/psemdel/FirstCyclingAPI/blob/main/tests/test_race.py

baronet2 commented 1 year ago

Without the class Standings, you would have to make a try/except everywhere, as sometimes your result would be in "result_tables" (for old races) and sometimes directly at the root (for new ones).

I understand. However, with your method we actually lose the ability to load all the classifications at once for the new races. For example, previously, you could do:

basque.edition(year = 2023).results(classification_num = 2).standings['points']

but I think in your PR that data will no longer be exposed.

Furthermore, the try/except situation is not too bad, since users can use the year to know which "style" it is (2023 is new, <2022 is old), and we have a warning in place to let them know about this quirk.

Because of these reasons, I don't think it is worth adding the Standings class. I hope you see my point of view.

Would be happy to accept PRs fixing the other issues, such as the startlist endpoint.

psemdel commented 1 year ago

Ok, it is your repo ;) Let's see how much adaption it requires without this one.

psemdel commented 1 year ago

I removed the standings, but frankly you can see it yourself in the test: sometimes there is results_table, sometimes not. How to plan that... in combi.py I have also strange if everywhere to check if results_table is there...

baronet2 commented 1 year ago

@psemdel , I hope I am not frustrating you. Don't get me wrong - I really appreciate your support. The startlist parser will be very useful, and I'll have a closer look in about a week or two, as I am travelling again.

sometimes there is results_table, sometimes not. How to plan that...

I have in mind something like:

if race.year >= 2023:
  race.results(classification_num = 2).standings['youth']
else:
  race.results(classification_num = 2).results_table

Do you think that would be possible?

Also, I see that you have added some new changes. Are those meant to be in a separate PR or do they relate to the Standings change? It looks to me like the Standings stuff is still in this PR.