From ad1d244fbf87143a089166f569869ef56a8460b7 Mon Sep 17 00:00:00 2001 From: rpessoa Date: Mon, 4 Nov 2024 11:29:42 +0000 Subject: [PATCH 1/3] [CHANGE] Updates typing & fixes some MyPy issues - AIL-44 --- .../assessment/standalone_assessment.py | 6 ++--- learnosity_sdk/request/init.py | 15 ++++++------- pyproject.toml | 3 +++ tests/integration/test_dataapi.py | 22 ++++++++++--------- tests/unit/test_dataapi.py | 21 ++++++++++-------- tests/unit/test_init.py | 9 ++++---- tests/unit/test_sdk.py | 15 +++++++------ tests/unit/test_uuid.py | 4 +++- 8 files changed, 53 insertions(+), 42 deletions(-) create mode 100644 pyproject.toml diff --git a/docs/quickstart/assessment/standalone_assessment.py b/docs/quickstart/assessment/standalone_assessment.py index cacca7d..deec11d 100644 --- a/docs/quickstart/assessment/standalone_assessment.py +++ b/docs/quickstart/assessment/standalone_assessment.py @@ -322,7 +322,7 @@ # Set up the HTML page template, for serving to the built-in Python web server class LearnosityServer(BaseHTTPRequestHandler): - def createResponse(self,response): + def createResponse(self, response: str) -> None: # Send headers and data back to the client. self.send_response(200) self.send_header("Content-type", "text/html") @@ -330,7 +330,7 @@ def createResponse(self,response): # Send the response to the client. self.wfile.write(response.encode("utf-8")) - def do_GET(self): + def do_GET(self) -> None: if self.path.endswith("/"): @@ -542,7 +542,7 @@ def do_GET(self): self.createResponse(response) -def main(): +def main() -> None: web_server = HTTPServer((host, port), LearnosityServer) print("Server started http://%s:%s. Press Ctrl-c to quit." % (host, port)) try: diff --git a/learnosity_sdk/request/init.py b/learnosity_sdk/request/init.py index cab08fd..5afe19c 100644 --- a/learnosity_sdk/request/init.py +++ b/learnosity_sdk/request/init.py @@ -35,13 +35,12 @@ class Init(object): def __init__( self, service: str, security: Dict[str, Any], secret: str, - request: Optional[Dict[str, Any]] = None, action:Optional[str] = None) -> None: - # Using None as a default value will throw mypy typecheck issues. This should be addressed + request: Optional[Union[Dict[str, Any], str]] = None, action:Optional[str] = None) -> None: self.service = service self.security = security.copy() self.secret = secret self.request = request - if hasattr(request, 'copy'): + if request is not None and hasattr(request, 'copy'): self.request = request.copy() self.action = action @@ -193,7 +192,7 @@ def set_service_options(self) -> None: elif self.service == 'assess': self.sign_request_data = False - if 'questionsApiActivity' in self.request: + if self.request is not None and 'questionsApiActivity' in self.request: questionsApi = self.request['questionsApiActivity'] if 'domain' in self.security: @@ -223,14 +222,14 @@ def set_service_options(self) -> None: self.request['questionsApiActivity'].update(questionsApi) elif self.service == 'items' or self.service == 'reports': - if 'user_id' not in self.security and \ - 'user_id' in self.request: + if self.request is not None and ('user_id' not in self.security and 'user_id' in self.request): self.security['user_id'] = self.request['user_id'] elif self.service == 'events': self.sign_request_data = False hashed_users = {} - for user in self.request.get('users', []): + users = self.request.get('users', []) if self.request is not None else [] + for user in users: concat = "{}{}".format(user, self.secret) hashed_users[user] = hashlib.sha256(concat.encode('utf-8')).hexdigest() @@ -244,7 +243,7 @@ def hash_list(self, l: Iterable[Any]) -> str: return '$02$' + signature def add_telemetry_data(self) -> None: - if self.__telemetry_enabled: + if self.request is not None and self.__telemetry_enabled: if 'meta' in self.request: self.request['meta']['sdk'] = self.get_sdk_meta() else: diff --git a/pyproject.toml b/pyproject.toml new file mode 100644 index 0000000..31b2311 --- /dev/null +++ b/pyproject.toml @@ -0,0 +1,3 @@ +[tool.mypy] +strict = true +files = ["learnosity_sdk", "docs", "tests"] diff --git a/tests/integration/test_dataapi.py b/tests/integration/test_dataapi.py index bfd5a42..e6b08fe 100644 --- a/tests/integration/test_dataapi.py +++ b/tests/integration/test_dataapi.py @@ -1,3 +1,4 @@ +from typing import Any, Dict, List, cast import unittest import os from learnosity_sdk.request import DataApi @@ -33,7 +34,7 @@ class IntegrationTestDataApiClient(unittest.TestCase): @staticmethod - def __build_base_url(): + def __build_base_url() -> str: env = os.environ env_domain = '' region_domain = '.learnosity.com' @@ -52,7 +53,7 @@ def __build_base_url(): return base_url - def test_real_request(self): + def test_real_request(self) -> None: """Make a request against Data Api to ensure the SDK works""" client = DataApi() res = client.request(self.__build_base_url() + items_endpoint, security, consumer_secret, items_request, @@ -62,9 +63,10 @@ def test_real_request(self): assert len(returned_json['data']) > 0 returned_ref = returned_json['data'][0]['reference'] - assert returned_ref in items_request['references'] + references: List[str] = cast(List[str], items_request['references']) + assert returned_ref in references - def test_paging(self): + def test_paging(self) -> None: """Verify that paging works""" client = DataApi() pages = client.request_iter(self.__build_base_url() + items_endpoint, security, consumer_secret, @@ -78,14 +80,14 @@ def test_paging(self): assert len(results) == 2 assert results == {'item_2', 'item_3'} - def test_real_request_with_special_characters(self): + def test_real_request_with_special_characters(self) -> None: """Make a request against Data Api to ensure the SDK works""" client = DataApi() # Add a reference containing special characters to ensure # signature creation works with special characters in the request - local_items_request = items_request.copy() # prevent modifying the base fixture - local_items_request['references'] = items_request['references'].copy() # prevent modifying the base fixture's list + local_items_request: Dict[str, Any] = items_request.copy() # prevent modifying the base fixture + local_items_request['references'] = cast(List[str], items_request['references'])[:] # prevent modifying the base fixture's list local_items_request['references'].append('ั‚ะตัั‚') res = client.request(self.__build_base_url() + items_endpoint, security, consumer_secret, items_request, @@ -95,9 +97,9 @@ def test_real_request_with_special_characters(self): assert len(returned_json['data']) > 0 returned_ref = returned_json['data'][0]['reference'] - assert returned_ref in items_request['references'] + assert returned_ref in cast(List[str], items_request['references']) - def test_real_question_request(self): + def test_real_question_request(self) -> None: """Make a request against Data Api to ensure the SDK works""" client = DataApi() @@ -114,7 +116,7 @@ def test_real_question_request(self): assert keys == {'py-sdk-test-2019-1', 'py-sdk-test-2019-2'} - def test_question_paging(self): + def test_question_paging(self) -> None: """Verify that paging works""" client = DataApi() diff --git a/tests/unit/test_dataapi.py b/tests/unit/test_dataapi.py index 68fad2a..40fed73 100644 --- a/tests/unit/test_dataapi.py +++ b/tests/unit/test_dataapi.py @@ -1,3 +1,4 @@ +from typing import Any, Dict, cast import unittest import responses from learnosity_sdk.request import DataApi @@ -8,7 +9,7 @@ class UnitTestDataApiClient(unittest.TestCase): Tests to ensure that the Data API client functions correctly. """ - def setUp(self): + def setUp(self) -> None: # This test uses the consumer key and secret for the demos consumer # this is the only consumer with publicly available keys self.security = { @@ -44,7 +45,7 @@ def setUp(self): self.invalid_json = "This is not valid JSON!" @responses.activate - def test_request(self): + def test_request(self) -> None: """ Verify that `request` sends a request after it has been signed """ @@ -55,10 +56,10 @@ def test_request(self): self.action) assert res.json() == self.dummy_responses[0] assert responses.calls[0].request.url == self.endpoint - assert 'signature' in responses.calls[0].request.body + assert 'signature' in cast(Dict[str, Any], responses.calls[0].request.body) @responses.activate - def test_request_iter(self): + def test_request_iter(self) -> None: """Verify that `request_iter` returns an iterator of pages""" for dummy in self.dummy_responses: responses.add(responses.POST, self.endpoint, json=dummy) @@ -74,7 +75,7 @@ def test_request_iter(self): assert results[1]['data'][0]['id'] == 'b' @responses.activate - def test_results_iter(self): + def test_results_iter(self) -> None: """Verify that `result_iter` returns an iterator of results""" self.dummy_responses[1]['data'] = {'id': 'b'} for dummy in self.dummy_responses: @@ -89,7 +90,7 @@ def test_results_iter(self): assert results[1]['id'] == 'b' @responses.activate - def test_results_iter_error_status(self): + def test_results_iter_error_status(self) -> None: """Verify that a DataApiException is raised http status is not ok""" for dummy in self.dummy_responses: responses.add(responses.POST, self.endpoint, json={}, status=500) @@ -99,10 +100,12 @@ def test_results_iter_error_status(self): self.request, self.action)) @responses.activate - def test_results_iter_no_meta_status(self): + def test_results_iter_no_meta_status(self) -> None: """Verify that a DataApiException is raised when 'meta' 'status' is None""" for response in self.dummy_responses: - response['meta']['status'] = None + # This is for typing purposes only, and should always be True + if isinstance(response['meta'], dict): + response['meta']['status'] = None for dummy in self.dummy_responses: responses.add(responses.POST, self.endpoint, json=dummy) @@ -112,7 +115,7 @@ def test_results_iter_no_meta_status(self): self.request, self.action)) @responses.activate - def test_results_iter_invalid_response_data(self): + def test_results_iter_invalid_response_data(self) -> None: """Verify that a DataApiException is raised response data isn't valid JSON""" for dummy in self.dummy_responses: responses.add(responses.POST, self.endpoint, json=None) diff --git a/tests/unit/test_init.py b/tests/unit/test_init.py index cd12dc0..0c7654b 100644 --- a/tests/unit/test_init.py +++ b/tests/unit/test_init.py @@ -1,10 +1,11 @@ import collections +from typing import Dict, Optional import unittest import learnosity_sdk.request ServiceTestSpec = collections.namedtuple( - "TestSpec", [ + "ServiceTestSpec", [ "service", "valid", "security", # security can be None to use the default, or an Dict to extend the default @@ -111,7 +112,7 @@ class TestServiceRequests(unittest.TestCase): domain = 'localhost' timestamp = '20140626-0528' - def test_init_generate(self): + def test_init_generate(self) -> None: """ Test that Init.generate() generates the desired initOptions """ @@ -125,7 +126,7 @@ def test_init_generate(self): self.assertFalse(init.is_telemetry_enabled(), 'Telemetry still enabled') self.assertEqual(t.signature, init.generate_signature(), 'Signature mismatch') - def test_no_parameter_mangling(self): + def test_no_parameter_mangling(self) -> None: """ Test that Init.generate() does not modify its parameters """ learnosity_sdk.request.Init.enable_telemetry() for t in ServiceTests: @@ -143,7 +144,7 @@ def test_no_parameter_mangling(self): self.assertEqual(security, security_copy, 'Original security modified by SDK') self.assertEqual(t.request, request_copy, 'Original request modified by SDK') - def _prepare_security(self, add_security=None): + def _prepare_security(self, add_security: Optional[Dict[str, str]]=None) -> Dict[str, str]: # TODO(cera): Much more validation security = { 'consumer_key': self.key, diff --git a/tests/unit/test_sdk.py b/tests/unit/test_sdk.py index 3ff7774..ee4657b 100644 --- a/tests/unit/test_sdk.py +++ b/tests/unit/test_sdk.py @@ -1,8 +1,9 @@ import collections +from typing import Any, Dict import unittest SdkTestSpec = collections.namedtuple( - "TestSpec", ["import_line", "object"]) + "SdkTestSpec", ["import_line", "object"]) SdkImportTests = [ SdkTestSpec( @@ -34,9 +35,9 @@ ), ] -def _run_test(t): - globals = {} - locals = {} +def _run_test(t: Any) -> None: + globals: Dict[str, Any] = {} + locals: Dict[str, Any] = {} exec(t.import_line, globals, locals) eval(t.object, globals, locals) @@ -45,15 +46,15 @@ class TestSdkImport(unittest.TestCase): Tests importing the SDK """ - def test_sdk_imports(self): + def test_sdk_imports(self) -> None: for t in SdkImportTests: _run_test(t) -class TestSdkImport(unittest.TestCase): +class TestSdkModuleImport(unittest.TestCase): """ Tests importing the modules """ - def test_sdk_imports(self): + def test_sdk_imports(self) -> None: for t in SdkModuleTests: _run_test(t) diff --git a/tests/unit/test_uuid.py b/tests/unit/test_uuid.py index 61bb40a..e6c78ac 100644 --- a/tests/unit/test_uuid.py +++ b/tests/unit/test_uuid.py @@ -1,9 +1,10 @@ +from typing import cast import unittest import re from learnosity_sdk.utils import Uuid class TestUuid(unittest.TestCase): - def test_generate(self): + def test_generate(self) -> None: """ Tests correctness of the generate() method in Uuid. """ @@ -13,4 +14,5 @@ def test_generate(self): result = prog.match(generated) assert result != None + result = cast(re.Match[str], result) assert result.group() == generated From 9eb2674a33c4fc61c42a87458915d178c4cc2923 Mon Sep 17 00:00:00 2001 From: rpessoa Date: Mon, 4 Nov 2024 11:46:09 +0000 Subject: [PATCH 2/3] [FEATURE] Adds type checking to github CI - AIL-44 --- .github/workflows/ci.yml | 17 +++++++++++++++++ ChangeLog.md | 1 + setup.py | 2 ++ 3 files changed, 20 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 439af62..81d4f07 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,6 +23,23 @@ jobs: - name: Run Pre-commit Hooks run: pre-commit run --all-files + type_check: + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.9' + + - name: Install Mypy Dependencies + run: pip install mypy + + - name: Type Checking with Mypy + run: mypy + tests: runs-on: ubuntu-latest strategy: diff --git a/ChangeLog.md b/ChangeLog.md index 2989ef3..7ca701b 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [unreleased] - 2024-11-01 ### Added - Added pre-commit hooks and Github CI action for code formatting and linting. +- Added MyPy with strict settings to enforce type hints (and Github CI action). ## [v0.3.11] - 2024-11-01 ### Fixed diff --git a/setup.py b/setup.py index 364eec3..26e873c 100644 --- a/setup.py +++ b/setup.py @@ -24,6 +24,8 @@ 'pytest-cov >=2.8.1', 'pytest-subtests', 'responses >=0.8.1', + 'types-requests', + 'mypy', ] # Extract the markdown content of the README to be sent to Pypi as the project description page. From 1af8eca7be5f1cbc2e13d90ce244667ad11b585b Mon Sep 17 00:00:00 2001 From: rpessoa Date: Tue, 5 Nov 2024 10:53:02 +0000 Subject: [PATCH 3/3] [CHANGE] Implements code review changes - AIL-44 --- .github/workflows/ci.yml | 4 ++-- learnosity_sdk/request/init.py | 3 ++- setup.py | 1 + tests/unit/test_uuid.py | 4 +--- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 81d4f07..d43b433 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,8 +34,8 @@ jobs: with: python-version: '3.9' - - name: Install Mypy Dependencies - run: pip install mypy + - name: Install Test Dependencies + run: pip install .[test] - name: Type Checking with Mypy run: mypy diff --git a/learnosity_sdk/request/init.py b/learnosity_sdk/request/init.py index 5afe19c..1f0ead1 100644 --- a/learnosity_sdk/request/init.py +++ b/learnosity_sdk/request/init.py @@ -40,7 +40,8 @@ def __init__( self.security = security.copy() self.secret = secret self.request = request - if request is not None and hasattr(request, 'copy'): + # TODO: Fix improper handling when request is a string + if isinstance(request, dict): self.request = request.copy() self.action = action diff --git a/setup.py b/setup.py index 26e873c..f175d54 100644 --- a/setup.py +++ b/setup.py @@ -25,6 +25,7 @@ 'pytest-subtests', 'responses >=0.8.1', 'types-requests', + 'types-Jinja2', 'mypy', ] diff --git a/tests/unit/test_uuid.py b/tests/unit/test_uuid.py index e6c78ac..222763e 100644 --- a/tests/unit/test_uuid.py +++ b/tests/unit/test_uuid.py @@ -1,4 +1,3 @@ -from typing import cast import unittest import re from learnosity_sdk.utils import Uuid @@ -13,6 +12,5 @@ def test_generate(self) -> None: prog = re.compile('[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}') result = prog.match(generated) - assert result != None - result = cast(re.Match[str], result) + assert result is not None assert result.group() == generated