Conversation
jacalata
commented
Oct 8, 2025
- added explicit checks for None to avoid union typing
- created a union for RequestOptionsType
- made static methods into class methods
- created a superclass for tests
c:\dev\tabcmd>mypy tests tests\commands\test_projects_utils.py:22: error: Argument 1 to "_parse_project_path_to_list" of "Server" has incompatible type "None"; expected "str" [arg-type] tests\commands\test_user_utils.py:76: error: Incompatible types in assignment (expression has type "UserItem | None", variable has type "UserItem") [assignment] tests\commands\test_user_utils.py:84: error: Incompatible types in assignment (expression has type "UserItem | None", variable has type "UserItem") [assignment] tests\e2e\language_tests.py:208: error: "OnlineCommandTest" has no attribute "_get_workbook" [attr-defined] tests\e2e\tests_integration.py:68: error: Name "logger" is not defined [name-defined] tests\e2e\tests_integration.py:99: error: Name "logger" is not defined [name-defined] tests\e2e\tests_integration.py:133: error: Name "__class__" is not defined [name-defined] tests\commands\test_session.py:206: error: Argument 1 to "_allow_prompt" has incompatible type "Namespace"; expected "Session" [arg-type] tests\commands\test_session.py:211: error: Argument 1 to "_allow_prompt" has incompatible type "Namespace"; expected "Session" [arg-type] tests\commands\test_session.py:216: error: Argument 1 to "_allow_prompt" has incompatible type "Namespace"; expected "Session" [arg-type] Found 10 errors in 5 files (checked 51 source files)
output with fixes > mypy tests Success: no issues found in 51 source files
There was a problem hiding this comment.
Pull request overview
This pull request enables mypy's check_untyped_defs configuration to improve type safety across the codebase. The changes focus on adding explicit type annotations, converting static methods to class methods, and improving None-safety checks to satisfy mypy's stricter type checking requirements.
Changes:
- Added explicit type annotations throughout the codebase (Optional types, union types, return types)
- Converted
@staticmethoddecorators to@classmethodin command classes, changing logger initialization from__class__.__name__tocls.__name__ - Introduced
ParserTestbase class for parser tests to provide consistent test infrastructure - Updated
Errors.exit_with_errorcalls to use namedexceptionparameter consistently - Added defensive None checks and assertions to prevent AttributeError in code paths where Optional types are used
Reviewed changes
Copilot reviewed 72 out of 72 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Enabled check_untyped_defs = true in mypy configuration |
| tests/parsers/common_setup.py | Created ParserTest base class with type annotations for test infrastructure |
| tests/parsers/test_parser_*.py (multiple files) | Updated test classes to inherit from ParserTest instead of unittest.TestCase |
| tests/parsers/test_parser_add_user.py | Fixed assertion syntax by removing extraneous ", args" expressions |
| tests/parsers/test_parser_get_url.py | Changed mock_args from dict to list for proper argument parsing |
| tests/parsers/test_login_parser.py | Added type annotation for commandname variable |
| tests/commands/test_user_utils.py | Added Optional type hints and None assertions for _parse_line results |
| tests/commands/test_session.py | Fixed test methods to properly call instance method _allow_prompt; corrected site to site_id |
| tests/commands/test_run_commands.py | Changed ServerResponseError code parameter from int to string; added missing import |
| tests/commands/test_projects_utils.py | Removed test case for None input to _parse_project_path_to_list |
| tests/commands/test_geturl_utils.py | Fixed string escaping in test URL |
| tests/e2e/tests_integration.py | Moved logger instantiation to module level |
| tests/e2e/language_tests.py | Added _get_workbook helper method |
| tabcmd/tabcmd.py | Added defensive None check for traceback to prevent AttributeError |
| tabcmd/execution/parent_parser.py | Changed parser from class attribute to properly typed instance attribute |
| tabcmd/commands/auth/session.py | Added Optional type annotations for all session attributes; added None checks with asserts |
| tabcmd/commands/auth/login_command.py | Converted run_command from static to class method |
| tabcmd/commands/auth/logout_command.py | Converted run_command from static to class method |
| tabcmd/commands/server.py | Updated method signature to accept Optional parameters |
| tabcmd/commands/user/*.py (multiple files) | Converted run_command from static to class methods |
| tabcmd/commands/user/create_site_users.py | Changed error message handling (incomplete commented code) |
| tabcmd/commands/user/add_users_command.py | Added exception handling with e.cause logging |
| tabcmd/commands/site/*.py (multiple files) | Converted run_command methods; updated exit_with_error calls |
| tabcmd/commands/project/*.py (multiple files) | Converted run_command methods; updated exit_with_error calls |
| tabcmd/commands/group/*.py (multiple files) | Converted run_command methods; updated exit_with_error calls |
| tabcmd/commands/extracts/*.py (multiple files) | Converted run_command methods; moved type annotations outside conditionals |
| tabcmd/commands/datasources_and_workbooks/*.py (multiple files) | Added RequestOptionsType union; converted methods; changed init to pass |
| tabcmd/commands/datasources_and_workbooks/datasources_workbooks_views_url_parser.py | Added None check for custom_view_item.view |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __init__(self, args): | ||
| super().__init__(args) | ||
| pass |
There was a problem hiding this comment.
The init method now only contains 'pass' and ignores the 'args' parameter. This breaks the parent class contract if Server.init expects these arguments. Since this class inherits from Server and Server doesn't have an init, this change makes the init unnecessary. Consider removing the init method entirely rather than having an empty implementation that accepts but ignores parameters.
|
|
||
| def test_parse_get_url_to_view_parts_without_slashes(self): | ||
| url = "views\wb name\\view-name" | ||
| url = "views\\wb name\\view-name" |
There was a problem hiding this comment.
The string has an incorrect escape sequence. It should be "views\\wb name\\view-name" (with double backslashes for proper escaping) or a raw string r"views\wb name\view-name". The current form "views\wb name\view-name" will be interpreted with a single backslash which is the intended behavior, but it's inconsistent with typical Python string escaping conventions for test data.
| number_of_errors += 1 | ||
| logger.debug(number_of_errors) | ||
| error_list.append(e.summary + ": " + e.detail) | ||
| error_list.append(e.__class__.__name__) # + ": " + e.__cause__ or "Unknown") |
There was a problem hiding this comment.
This line appears to be incomplete and contains commented-out code. The comment suggests building an error message with "e.cause" but this is left incomplete with "+ ": " + e.cause or "Unknown")". Either complete the error message construction or remove the comment entirely. Additionally, just appending the class name without any additional context may not provide enough information for debugging.
| @@ -1,33 +1,43 @@ | |||
| import unittest | |||
There was a problem hiding this comment.
Module 'unittest' is imported with both 'import' and 'import from'.
| @@ -1,33 +1,43 @@ | |||
| import unittest | |||
| from unittest import mock | |||
There was a problem hiding this comment.
Import of 'mock' is not used.
| from tabcmd.execution import parent_parser | ||
| from collections import namedtuple | ||
| import argparse | ||
| from typing import Optional, Type, Any |
There was a problem hiding this comment.
Import of 'Optional' is not used.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>