-
-
Notifications
You must be signed in to change notification settings - Fork 97
Made GitHub cache update async and fixed cache refresh check. Fixes #… #1316
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
Conversation
Hey @Zabuzard, could you please review this PR? |
Great stuff, thanks. I'll have a closer look later but looks good so far 👍 |
Hey @Alathreon, I guess #1317 means this fix is not needed anymore? so, should i close this? Pretty neat solution btw. |
Hello @waliamehak |
We want both. The code being blocking is not "correct". The page size being suboptimal isnt good either. Both have to be tackled :) |
application/src/main/java/org/togetherjava/tjbot/features/github/GitHubCommand.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/github/GitHubCommand.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/github/GitHubCommand.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/github/GitHubCommand.java
Outdated
Show resolved
Hide resolved
Hi @Zabuzard, updated the commit accordingly. Let me know if everything is up to speed now :) |
FYI after a code review has started, force-push should not be done anymore. The problem is that a force-push changes commit history. So GitHub cannot tell me now anymore which commits I already reviewed and which contain actual new changes. Like, If I click on "rewiew commits since my last review" I now get this: So I have to essentially restart my review from scratch now, instead of doing it incremental. Fortunately, this PR is relatively small so its not that big of a deal. But you gotta keep that in mind 👍 Better not do any force-push after you received a CR anymore and just do regular merges at that point 🙂 |
@waliamehak Looks like the file has a minor spotless/style issue. Running |
Thanks, @Zabuzard, I'll avoid force pushes in the future. Also, Spotless fails on some pre-existing _ identifiers, these are not changes from this PR so ignoring these so we can proceed with review; all other formatting issues are clean. |
Only after a CR. Before a CR they are completely fine 👍
Not on my end, mh... Sounds like you are potentially on the wrong Java version somewhere, as this was a very recent addition.
Great, thanks for your efforts! 🙂 |
@Zabuzard, you are right. Switching to JDK 24 resolves them :) |
Fixes:
Also did: