search

LennysLounge / ACC-Race-Control

A live timing app for Assett Corsa Competizione
MIT License
31 stars 5 forks source link

Fix sector times on Qualifying Best & Last screens #21

Closed NoComment1105 closed 3 months ago

NoComment1105 commented 3 months ago

Also does updates to dependencies and code

LennysLounge commented 3 months ago

Hello and thanks for contributing.

This seams to be a small and reasonable change but i do have to wonder why this PR is so big. In general PRs should be as small as possible and as large as necessary. In this PR i see a lot of changes that are not immediately obvious as to why they are necessary.

1) I guess you use Intellij? The .idea directory should not be checked in with this repo because it should stay agnostic to the IDE used to develop the project. Ideally it would be in the git ignore file but i dont use IntelliJ which is why it isnt there. Feel free to add it.

2) Why were the dependecies updated? Im not against it but there should be a good reason for doing so. And if dependencies are updated that really should be happening in a seperate PR to keep features/bugs seperate from project maintenance.

3) You have changed the license header in multiple files; do not do this. I understand that this is likely an encoding issue. Please fix.

NoComment1105 commented 3 months ago

I do indeed use IntelliJ, I'll add it to the .gitignore. IntelliJ informed me there were many vulnerabilities in the old dependencies, so I thought a general update would be ideal. Encoding go brrr, I'll sort it out. Also when I used the Start.bat file it wouldn't properly open the application, I might be being dense but if you can reproduce that behavior it would be great

NoComment1105 commented 3 months ago

Sorted those things out for you 👍🏼

LennysLounge commented 3 months ago

@NoComment1105 , With the new Nürburgring update i made some fixed and included your fix in there to. I thought i would be easier if i copy the necessary changed directly so we dont have to go back and forth. I also mentioned your github handle in the release notes.

NoComment1105 commented 3 months ago

All good my friend, thanks for fixing it. I understand they way you've done it!