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

fix: Show correct time until next match #147

Merged
merged 8 commits into from
Feb 22, 2023
Merged

fix: Show correct time until next match #147

merged 8 commits into from
Feb 22, 2023

Conversation

MJ-GH
Copy link
Contributor

@MJ-GH MJ-GH commented Feb 15, 2023

Fixes #124

Summary of Changes

Properly displays time until next match in the Live Matches column.

Additional context

Discord username (if different from GitHub): Frozty

Testing instructions

Sometimes it won't be able to set a time because the json response is not up to date. Usually happens right as a match goes live and the event will show 'state': 'unstarted' when its actually live(should be 'state': 'inProgress'). The issue resolves itself as the data is updated. It gave me a headache during testing, so just keep it in mind.

How to download the PR for testing

Using GitHub CLI

  1. Clone this PR
  2. Run gh pr checkout 124 (Requires GitHub CLI)
  3. Follow the Advanced Installation Guides from the Wiki

Using regular GIT

  1. Fetch the PR git fetch origin pull/<PR_NUMBER>/head:<LOCAL_BRANCH_NAME> (e.g. git fetch origin pull/110/head:notif)
  2. Checkout the branch git checkout <LOCAL_BRANCH_NAME> (e.g. git checkout notif)
  3. Follow the Advanced Installation Guides from the Wiki

@LeagueOfPoro
Copy link
Owner

Can someone test it please?

@Nub01
Copy link

Nub01 commented Feb 15, 2023

Seems to be working, I'm in PST, and next stream/game is in 56 mins,

fixes a rare edge case where some matches hadn't started when planned due to delay or cancellation
@MJ-GH
Copy link
Contributor Author

MJ-GH commented Feb 16, 2023

2839e5b fixes a rare edge case that happened this morning, where some matches were supposed to have started, but they didn't for some reason. My guess is delayed or cancellation. In case it happens again this commit will change the Live matches column to:
billede

Copy link
Collaborator

@alepouna alepouna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM till latest commit. I am unsure if that's going to cause issues. Haven't tested that part either.

@alepouna alepouna added the enhancement New feature or request label Feb 18, 2023
Copy link
Owner

@LeagueOfPoro LeagueOfPoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea: instead of the weird fix for a delayed match what about changing the message to something like "Up next: LEC at 19:00"

src/DataProviderThread.py Outdated Show resolved Hide resolved
src/DataProviderThread.py Outdated Show resolved Hide resolved
@MJ-GH
Copy link
Contributor Author

MJ-GH commented Feb 20, 2023

Idea: instead of the weird fix for a delayed match what about changing the message to something like "Up next: LEC at 19:00"

We'd be assuming everyone is in the same timezeone, if we write "19:00", that's why we calculate the time until the next match starts.
So the question is what do you want me to do with the else clause:

                    else:  # We're past the startTime due to delay or cancellation i'm guessing
                        niceStartTime = datetime.strftime(self.startTime, '%H:%M')
                        self.sharedData.setTimeUntilNextMatch(f"{event['league']['name']} should've started at {niceStartTime} but hasn't.")
                        for nextEvent in events:  # Looping again to find next match that weren't supposed to have started already
                            if nextEvent["state"] == "unstarted":
                                if self._isStartTimeLater(nextEvent["startTime"]):
                                    days, hours, minutes = self._calculateTime()
                                    self.sharedData.addTimeUntilNextMatch(
                                        f"Next in {days}d {hours}h" if days else f'Next in {hours}h {minutes}m')
                                    break

Thing is, if the if statement fails and we calculate time, then it will return -1 days. I could just have it say "None - Couldn't determine when next match is", like in my initial commit, or we loop through the events again like it does now, and calculate time once more. And then remove the "LEC should've started at 19:00" part. I can see why it's a weird fix for such a rare case, but it's up to you.

@Skribb11es
Copy link
Contributor

Couldn't you just use the system time + the time until the next game to format it correctly according to their timezone? Also shouldn't riot's APIs be returning the timezone-corrected start times anyhow?

@Nub01
Copy link

Nub01 commented Feb 20, 2023

Couldn't you just use the system time + the time until the next game to format it correctly according to their timezone? Also shouldn't riot's APIs be returning the timezone-corrected start times anyhow?

this is what wiflow tried doing isn't it?

@MJ-GH
Copy link
Contributor Author

MJ-GH commented Feb 20, 2023

Couldn't you just use the system time + the time until the next game to format it correctly according to their timezone? Also shouldn't riot's APIs be returning the timezone-corrected start times anyhow?

Riot's api doesn't do that, the start times are all UTC+0.

@MJ-GH
Copy link
Contributor Author

MJ-GH commented Feb 20, 2023

Idea: instead of the weird fix for a delayed match what about changing the message to something like "Up next: LEC at 19:00"

commit c29a502 makes it print a timezone-correct starttime no matter which timezone you're in, in the format you suggested.

@Skribb11es showed me on discord how easy that was. Thanks!

I also got rid of the weird fix with the nested for loop.

I'd like it tested in different timezones than mine(UTC+1) and if you don't want to wait for a real scanario, you can test this commit by adding this line to the end of the try clause between line 104 and 105.
print(self.sharedData.getTimeUntilNextMatch())
It should print this in the terminal with starttime in your timezone:
image
Check if the time matches the next match starttime on the schedule here: https://lolesports.com/schedule

@Skribb11es
Copy link
Contributor

LGTM, times match up with the correct timestamp for UTC-5, well done.

@DeadSpc
Copy link

DeadSpc commented Feb 21, 2023

image
help

@alepouna
Copy link
Collaborator

image help

You need to follow the advanced installation guide as the PR description shows, to test it locally.

@KitsumyFox
Copy link

image help

I think you just need pipenv install

@LeagueOfPoro LeagueOfPoro merged commit 2428f27 into LeagueOfPoro:master Feb 22, 2023
@MJ-GH MJ-GH deleted the fix branch February 22, 2023 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants