Skip to content

fix: fix match_params for bool#198

Open
bagowix wants to merge 10 commits intoColin-b:developfrom
bagowix:fix/197
Open

fix: fix match_params for bool#198
bagowix wants to merge 10 commits intoColin-b:developfrom
bagowix:fix/197

Conversation

@bagowix
Copy link

@bagowix bagowix commented Jan 19, 2026

@Colin-b
Copy link
Owner

Colin-b commented Jan 19, 2026

Hello, thanks for the proposal, if you can make the existing test suite pass I can review it. But for now some tests are failing, meaning this proposal introduce a regression.



@pytest.mark.asyncio
async def test_url_query_params_bool_matching(httpx_mock: HTTPXMock) -> None:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also test with a list of values? in a single param ? With bool, int and str values inside the list in the httpx query, and bool and str on httpx_mock matcher

params = {
k: (
primitive_value_to_str(v)
if isinstance(v, (str, int, float, bool))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I missed something, int and float are not tested

@bagowix
Copy link
Author

bagowix commented Jan 23, 2026

@Colin-b Hi!
I'm sorry for my haste and making stupid mistakes.
At the moment, I've gone through all your comments and fixed everything.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit testing an internal function does not ensure this code is actually reachable for clients. Could you write tests with those values the same way you wrote your other tests ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done)

@sonarqubecloud
Copy link

@bagowix
Copy link
Author

bagowix commented Jan 26, 2026

@Colin-b should I add # pragma: no cover ?

@bagowix
Copy link
Author

bagowix commented Jan 30, 2026

@Colin-b Hi! My question is still relevant =)

@Colin-b
Copy link
Owner

Colin-b commented Feb 1, 2026

@Colin-b should I add # pragma: no cover ?

There should be no need for it, it is most likely a test scenario missing or unreachable code. I'll check once I have some time. What I can also do, if you don't mind, is merge as is and refactor myself where needed. Up to you

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

Comments