Skip to content

Conversation

@sreyassabbani
Copy link
Contributor

See #8

@sreyassabbani
Copy link
Contributor Author

Oh no.

@sreyassabbani
Copy link
Contributor Author

I'm getting back to this, and I see a lot of repeated and possibly superfluous lines of code. For example, this in the query for both Sync and Async clients:

if not isinstance(queryString, str):
    raise TypeError(
        f"queryString must be a string, not {type(queryString).__name__}."
    )

However, the Python interpreter should be able to catch on to this, given the strict typing enforced in the function definition. I'm seeing this pattern throughout most methods in Sync and Async (although there are some reasonable numeric bound checks).

As for the repeated code, I see code being mirrored between qbreader/asynchronous.py and qbreader/synchronous.py. pylint points this out, and so should I move this repeated logic out to qbreader/_api_utils.py and remove the redundant code @geoffrey-wu?

@skysomorphic
Copy link
Member

Python is not statically-typed and the typing in the function definition are simply annotations that can be used by third-party programs such as language servers or optional static type checkers like mypy. The interpreter does not strictly enforce these typings at runtime, and will only raise exceptions further down when it runs into unexpected behavior. The relevant logic is to prevent things from ever getting to that stage and identifies exactly what is wrong (as opposed to perhaps a more confusing error later down), regardless if people are paying attention to the annotations or not.

I don't think the repeated code is too much of a problem for now, as Async and Sync use two completely different HTTP libraries, but I can definitely see some changes to that being made in the future. I would leave it for now.

@sreyassabbani
Copy link
Contributor Author

I completely forgot about that.

I did get working on just moving the duplicated logic already to _api_utils.py, but if you would rather me not do it, that's fine with me (although it kind of bugs me).

I'm going to try to get this fixed this soon, maybe sometime this week.

@geoffrey-wu geoffrey-wu merged commit dd1aa93 into qbreader:main Mar 7, 2025
8 checks 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.

3 participants