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. More … #27

Closed psemdel closed 1 year ago

psemdel commented 1 year ago

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

Thank you very much @psemdel ! I adapted your changes into a separate PR in #29 since I think it's a slightly easier solution. Feel free to review if you think there is anything missing there. If not, I'll go ahead and merge so we can move on to the next fix 😄

psemdel commented 1 year ago

Hum, I suppose some of your solutions are helping for sure. But for new stage races, we still have nothing in standings. And the l=01 / 02 will not work. What is your solution for that? As mentioned, some part of the code are also preparation for the PR2.

psemdel commented 1 year ago

I confirm that your change to headers and trs work also. (Can you merge your change, that I move further from there, it is too much chaos otherwise)

baronet2 commented 1 year ago

@psemdel sorry for the delay. I have merged my change now.

psemdel commented 1 year ago

Ok thanks, I need to give it some more 20 min, to be sure what is the right way :)

psemdel commented 1 year ago

I have still some progresses to do with git :( . I tested the new status and wanted to push a small correction, but I get permission denied.

In test_race, I need to change the last 2 lines (basque)

assert len(results_2023.standings['youth'].results_table) == 26
assert results_2023.standings['youth'].results_table['Rider'].iloc[0] == 'McNulty Brandon'

Note the results_table. I didn't introduce those for fun. I wanted to have the same way to handle standings and results. Otherwise, we will always need a condition to check if the other classification are in a results_table or without.

With your code with new races: r_2023.results(classification_num=2).results_table will return the gc, because the url redirect to the gc. So my idea was to use standings[] to handle that (and it works quite well). However here there is a "results_table", that's why I put it that way.

baronet2 commented 1 year ago

Thanks! I am on vacation for the next two weeks or so, but I will get back to your PR when I come back. If you want to set up other PRs for the other improvements, feel free to go ahead. I would just encourage you to make the PRs as small and straight-forward as possible, so it will be easy for me to understand and review.

Thanks for all your help!

psemdel commented 1 year ago

I let you finish this one, and then I will post more. The version I use has many changes in between ^^.

psemdel commented 1 year ago

Ah shit, I pushed my last changes to my repo and now it is all in this PR :(. It was not my intention.

baronet2 commented 1 year ago

If you would like, feel free to revert the recent changes and force-push. Just make sure you have them saved on another branch somewhere :)

baronet2 commented 1 year ago

@psemdel will you have a chance to revert the recent changes or should I try to get these fixes through myself?