-
-
Notifications
You must be signed in to change notification settings - Fork 264
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
Lap Integrity Check with previous lap "Time" #449
Conversation
Ignore test run failures for now. They are currently unrelated. |
Sorry that it took me so long to respond. I was busy and also travelling a bit. And this was not something I could just review on my phone 😅 Basically everything I criticized in the review is readability related. This is not completely on you, this block of code with multi-stage checks is already not very readable the way it is now. But it took me quite a while to understand what's going on with your additional 'flags' mixed into the other steps. Making the additional check self-contained should increase readability quite a bit. |
Good morning!!! Thank you for your precious suggestions. |
That looks much more readable already. I made one more suggestion regarding the duplicate integrity error for a single lap. But after that, I think this is good to merge. Edit: line length needs to be fixed though |
Good morning! I've resolved the conversations and fixed the line length of the code i wrote. Please note for the future that other lines of core.py needs to be fixed for line length, too. For any other issues please let me know. Thank you! |
Everything looks good now, thank you!
Yes, that's why the line length rule is currently only enforced on changes, so that the overall code base slowly moves towards proper line length. That's less intrusive than doing one huge commit that changes hundreds or thousands of lines throughout all files. |
Minor issue that came up during the discussion in #404. The control flags used in core.py apparently did not include checking the previous lap, but did include checking the integrity of the lap based on the sum of the 3 sectors against the corresponding "LapTime". I added the previous lap check mentioned earlier (apparently now it seems to mention integrity errors with the code above). If anything is not exact I am open to suggestions and/or improvements
After this possible implementation we could focus on the red flag bug issue