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

Laps-picking function improvements #376

Merged
merged 2 commits into from
Jun 5, 2023
Merged

Conversation

Casper-Guo
Copy link
Contributor

@Casper-Guo Casper-Guo commented May 13, 2023

Reference: #370

Additionally implemented any and none for the how argument of pick_track_status.

@Casper-Guo Casper-Guo marked this pull request as ready for review May 13, 2023 17:11
@theOehrly
Copy link
Owner

I'll try to review this soon. Ignore CI failures again, that problem should be fixed for new PRs though, I hope.

fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
@theOehrly
Copy link
Owner

I forgot one thing, all the docstrings for deprecated functions should get a .. deprecated:: note as well, so that this is already obvious from the documentation. It should basically contain the same info as the warning.

@Casper-Guo
Copy link
Contributor Author

I replaced all ff1.pick_* with session_laps.pick_*. I will squash the commits before merging.

fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
@theOehrly
Copy link
Owner

The .pick_* functions currently don't have dedicated tests and I don't want to ask you to create them for all.
But especially considering the added complexity in .pick_track_status and the mix of number and abbreviation supported in pick_drivers, it would be good if these to get tests.
In general, the policy here is, that new features should get tests (whenever reasonably possible).

The doc build failure seems to occur because the .. deprecated:: directive is used incorrectly. See https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-deprecated

@Casper-Guo
Copy link
Contributor Author

I was actually going to ask if you would like me to add tests anyways. I will write some in the next few days.

@Casper-Guo Casper-Guo force-pushed the pick-laps branch 3 times, most recently from 0bed205 to 51e9b5d Compare May 29, 2023 22:10
@Casper-Guo
Copy link
Contributor Author

Changed implementation of pick_compound to bring it in line with other picking functions. It also accepts a list of compound names now.

Modelled some tests after the existing test cases. Please let me know if you want them done another way.

@Casper-Guo Casper-Guo requested a review from theOehrly May 29, 2023 22:12
fastf1/testing/reference_values.py Outdated Show resolved Hide resolved
fastf1/tests/test_laps.py Outdated Show resolved Hide resolved
fastf1/tests/test_laps.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
@theOehrly
Copy link
Owner

Test failures are because of your changes (except documentation build).

@Casper-Guo Casper-Guo force-pushed the pick-laps branch 2 times, most recently from 8788053 to de8a4ae Compare June 4, 2023 21:14
@Casper-Guo
Copy link
Contributor Author

Linting problem outside of this PR

@theOehrly
Copy link
Owner

Looks good now. Do you have any further changes or remarks, or are you happy with everything?

@Casper-Guo
Copy link
Contributor Author

No more changes from me. Really appreciate your patience with this PR.

@theOehrly
Copy link
Owner

No more changes from me. Really appreciate your patience with this PR.

This took longer than I thought, yes. But that's just the way it ends up being sometimes. If you feel like I could have given better feedback on the way, please let me know. Like just about everything else on this project, reviewing PRs is something I learn as I go. Therefore, feel free to criticize and I'll try to improve in the future.

@theOehrly theOehrly merged commit 773979d into theOehrly:master Jun 5, 2023
@Casper-Guo Casper-Guo deleted the pick-laps branch June 5, 2023 18:18
jeffjose pushed a commit to jeffjose/Fast-F1 that referenced this pull request Jun 20, 2023
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