fix: resolve mypy type errors in cli/testflinger_cli/auth.py#1030
fix: resolve mypy type errors in cli/testflinger_cli/auth.py#1030Ahmedaltu wants to merge 1 commit intocanonical:mainfrom
Conversation
ajzobro
left a comment
ajzobro
left a comment
There was a problem hiding this comment.
Thanks for your offer to contribute to this project!
Please showcase how your code changes are non-breaking by including tests which ensure that the new code is still handling these conditions the same way. Especially cases where None has been replaced with "". See my comments below.
|
|
||
| # Set client_id from config file to keep token owner | ||
| self.client_id = config_file.get("AUTH", "client_id", fallback=None) | ||
| self.client_id = config_file.get("AUTH", "client_id", fallback="") |
There was a problem hiding this comment.
Why should client_id be set to an empty string instead of None here? I think that None is appropriate.
There was a problem hiding this comment.
Reverted to fallback=None. Added # type: ignore[arg-type] to satisfy mypy while keeping None as the intended value. No behaviour change.
| try: | ||
| decoded_jwt = jwt.decode( | ||
| self.jwt_token, options={"verify_signature": False} | ||
| self.jwt_token or "", options={"verify_signature": False} |
There was a problem hiding this comment.
Like above, rather than an empty string I feel like None is appropriate here as it ought to land us in the exception handler.
There was a problem hiding this comment.
Reverted to passing self.jwt_token directly. Added # type: ignore[arg-type] to satisfy mypy. None will still land in the exception handler as intended. No behaviour change.
Ahmedaltu
force-pushed
the
fix/auth-type-errors
branch
3 times, most recently
from
98632c2 to
675471e
Compare
|
Reverted both runtime changes. Added # type: ignore[arg-type] on lines 130 and 184 to satisfy mypy while keeping None as the intended value in both cases. No behaviour change. |
- Add return None after _handle_auth_error() calls in _authenticate_with_credentials and _authenticate_with_refresh_token - Add type: ignore[arg-type] on ConfigParser.get() fallback=None call - Add type: ignore[arg-type] on jwt.decode() call for str | None token Fixes canonical#1028 Signed-off-by: Ahmed Al-Tuwaijari <altuwaijari.ahmed@gmail.com>
Ahmedaltu
force-pushed
the
fix/auth-type-errors
branch
from
bd4ff00 to
e6d0eb3
Compare
2 participants
Summary
Fixes type errors in
cli/testflinger_cli/auth.pydetected by mypy.Changes
_authenticate_with_credentialsand_authenticate_with_refresh_token: addedreturn Noneafter_handle_auth_error()calls and updated return types fromstrtostr | Noneto reflect actual behaviourget_stored_refresh_token: changedfallback=Nonetofallback=""inConfigParser.get()call — ConfigParser expectsstr, notNonedecode_jwt_token: changedself.jwt_tokentoself.jwt_token or ""injwt.decode()call —jwt_tokenisstr | Nonebutjwt.decode()expectsstr | bytesVerification
Before:
cli/testflinger_cli/auth.py:79: error: Missing return statement [return]
cli/testflinger_cli/auth.py:91: error: Missing return statement [return]
cli/testflinger_cli/auth.py:126: error: Argument "fallback" to "get" of "ConfigParser" has incompatible type "None"; expected "str" [arg-type]
cli/testflinger_cli/auth.py:180: error: Argument 1 has incompatible type "str | None"; expected "str | bytes" [arg-type]
After:
(no errors for auth.py)
All 216 unit tests pass. tox checks pass (lock, format, lint, unit).
Closes #1028