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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

psemdel
Copy link

@psemdel psemdel commented Jun 17, 2023

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
Copy link
Owner

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
Copy link
Author

psemdel commented Jun 20, 2023

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
Copy link
Owner

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
Copy link
Author

psemdel commented Jun 22, 2023

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

@psemdel
Copy link
Author

psemdel commented Jun 22, 2023

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
Copy link
Owner

@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.

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