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

V0.4.0 release #98

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

V0.4.0 release #98

wants to merge 21 commits into from

Conversation

j-andrews7
Copy link
Owner

Rolls in fixes to #90, #94, #92, and #93 from @seankim658.

@esqew, if you want to take a look, this should make it easy. Most stuff looks fine to me, but I'll check with a closer eye when I get a chance. cloudflare stuff is the biggest concern.

@seankim658
Copy link

Looks like lxml will need to be added to the requirements.txt. It was previously installed as a dependency when installing mechanicalsoup. But it isn't a dependency for pandas or cloudscraper so it'll have to be explicitly listed in the requirements file.

Also, another issue that cropped up this morning, kenpom updated the "Sorry, no games today" fanmatch message from "Sorry, no games today :(" to "Sorry, no games today ☹" (using an emoji). So I'll open a new PR to fix both of these issues.

Adds `lxml` and fix for new no games fanmatch message
@seankim658
Copy link

seankim658 commented Nov 4, 2024

I was developing/testing on ubuntu and pop os so makes sense those tests passed. I don't currently have a windows machine so not really too sure where to go for the failures there. Curious as to whether the MacOS tests would have passed. It looks like cloudscraper's create_scraper function can take a dictionary parameter where you can try to specify the platform. Not sure if this would work. Interesting that with default parameters it works from Ubuntu but fails from windows.

@j-andrews7
Copy link
Owner Author

Yeah, we've also had just periodic failures of tests from Github actions due to login problems that sometimes would work on retry (which is what I'm doing now).

If it fails again, will dig deeper.

@j-andrews7
Copy link
Owner Author

Yeah, so the first Windows set of tests passed on python 3.8, then failed on 3.9 due to the login issue. But on repeat, the 3.9 tests are now running.

Frankly, may just catch the login issue in the tests and run it again if it hits.

@j-andrews7
Copy link
Owner Author

Not sure you can see this, but the fanmatch test is failing on MacOS.

https://github.com/j-andrews7/kenpompy/actions/runs/11656227363/job/32490372111

Not sure why, seems like it's still pulling data even with a date with no games.

@seankim658
Copy link

It seems like a lot of small things are changing on the site for the start of the season. I have a script that has been pulling the fanmatch html everyday (how I caught the previous fanmatch change) and it looks like sometime in the past 12 ish hours the "Time" column has changed. If we re-run the Ubuntu and Windows tests they would fail. The tag used to point to a link that included the actual day the games were on, which is how the date checking was done before. Unfortunately that has been removed in favor of a simpler sort link that look like this: <a href="fanmatch.php?s=Time">Time (ET)</a>. So I'll have to rework that logic to parse the title text which is in "%B %d" format. I'll try to get this pushed and open a new PR tonight. Hopefully now that the season is under way the site will be more stable.

@seankim658
Copy link

seankim658 commented Nov 4, 2024

Also noticed that in the Prediction column that there's a new formatting for posessions:

image

This is throwing off the existing regex matching so this will also have to be fixed.

@j-andrews7
Copy link
Owner Author

Tests in actions did eventually pass after enough retries.

Probably won't have time for a real review/release till the weekend.

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