Skip to content

Conversation

@abelsm2
Copy link
Contributor

@abelsm2 abelsm2 commented Aug 30, 2025

Previously, the payload builder filtered parameters using a truthiness check (if params[x]:). This unintentionally excluded valid values such as race_week_num=0 or official_only=False.

Updated the logic to only exclude parameters that are explicitly None, ensuring falsy but valid values are preserved in the payload. Added new test test_result_search_series_with_race_week_num_0() to verify fix.

Fixes #48.

@abelsm2
Copy link
Contributor Author

abelsm2 commented Aug 30, 2025

There is 1 failing test but that test failed when I forked the main branch and fails for an unrelated reason. I did not investigate the failure.

@abelsm2
Copy link
Contributor Author

abelsm2 commented Aug 30, 2025

it's also worth noting that there are other functions that built the payload in a similar way but I was not familiar with those end points to know what was valid and what wasn't, so I did not attempt to fix them.

@jasondilworth56
Copy link
Owner

Thanks @abelsm2, I appreciate the catch.

One thing I'd love to do at some point is properly add some type safety to the whole project – really define what goes in and out of each method. Just one of those things I never get around to, but would help to avoid bugs like this being possible.

I've taken a look at the other place that this happens and it could benefit from your same fix, would you mind putting it in place for that? I've also just merged #50, which will help to ensure you get some ticks on the tests flow at least.

@jasondilworth56
Copy link
Owner

On that note, it would be a decent shout to add some test coverage for this particular bug – ensure that things like your examples of race_week_num=0 or official_only=False make it through to the request.

@abelsm2
Copy link
Contributor Author

abelsm2 commented Aug 30, 2025

sure thing, I can add this fix to the other function that had the same bug. I did add a new test to verify that race_week_num=0 would pass. I can add a second test to verify the second fix.

@jasondilworth56
Copy link
Owner

Apologies for all the activity on here – I cannot understand why the labeller won't work. It's not particularly important, but it should just work.

@jasondilworth56
Copy link
Owner

Git can be a pain eh?!

Here's what I'd do if you want, although I'm far from expert.

On your local:

  1. If you don't have the main repo as a remote, do something like this:
  • git remote add upstream https://github.com/jasondilworth56/iracingdataapi.git
  1. git fetch upstream
  2. git checkout main
  3. git rebase upstream/main

Potentially you could change step 5 to:
5. git reset --soft upstream/main
6. commit your individual changes again
7. git push --force-with-lease

Either of those options should get you close to a nice clean commit history again without the conflicts.

@abelsm2
Copy link
Contributor Author

abelsm2 commented Sep 6, 2025

Yes, sorry about this. I admittedly have no idea what I'm doing when it comes to PR and most of the more interesting ways to deal with git. This repo is the 1st and only one I have ever contributed to where I was not the only developer. Hopefully the rebase and push will resolve the conflicts.

@jasondilworth56
Copy link
Owner

Don't you worry about it at all @abelsm2, up until relatively recently I was very similar - very much so when starting this repo too. It's only a change to a more specific professional role that's given me any clue at all.

I hope the difficulties of PRs like this are useful to you either way, but if it feels like a pain at some point let me know and I'm happy to just sort it out somehow.

No need to apologise at all, really appreciate the fact you bother to help 👍

@abelsm2
Copy link
Contributor Author

abelsm2 commented Sep 6, 2025

Nope, this is a good learning experience! Hopefully that last push got it.

@jasondilworth56 jasondilworth56 merged commit 90f7993 into jasondilworth56:main Sep 6, 2025
1 check passed
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.

Bug: payload excludes valid parameters in result_search_series()

2 participants