Feat/include both maxage and s maxage headers #60
Conversation
Reviewer's Guide by SourceryThis pull request implements the inclusion of both 'max-age' and 's-maxage' headers in the cache control logic. The changes involve updating the client methods to handle both headers, modifying the test cases to validate the new logic, and updating the models to include a new 'shared_expires' field. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @mnbf9rca - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| None, | ||
| None, | ||
| ), | ||
| # Edge case: zero timedelta |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test case for very large timedelta values
It would be beneficial to add a test case for very large timedelta values to ensure the function handles them correctly without overflow or performance issues.
| datetime(2023, 11, 15, 12, 45, 26), | ||
| datetime(2023, 11, 15, 12, 45, 26), | ||
| ), | ||
| # Negative timedelta |
There was a problem hiding this comment.
suggestion (testing): Add a test case for non-integer timedelta values
Consider adding a test case where the timedelta value is a non-integer (e.g., a float or a string) to ensure the function handles such inputs gracefully.
| "public, s-maxage=86400", | ||
| (86400, None), | ||
| ), | ||
| # Both max-age and s-maxage zero |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test case for mixed valid and invalid directives
It would be useful to add a test case where one directive is valid and the other is invalid (e.g., max-age is a valid number, but s-maxage is malformed) to ensure the function handles such mixed scenarios correctly.
| # Both max-age and s-maxage zero | |
| # Both max-age and s-maxage zero | |
| ( | |
| "public, max-age=0, s-maxage=0", | |
| (0, 0), | |
| ), | |
| # Mixed valid and invalid directives | |
| ( | |
| "public, max-age=3600, s-maxage=invalid", | |
| (3600, None), | |
| ), |
| ], | ||
| ) | ||
| def test_parse_timedelta(value, base_time, expected_result): | ||
| # Act |
There was a problem hiding this comment.
suggestion (testing): Consider adding assertions for exception handling
In the test cases where exceptions are expected (e.g., invalid input), consider adding assertions to check the exception message to ensure it is informative and accurate.
| # Act | |
| # Act & Assert | |
| if isinstance(expected_result, type) and issubclass(expected_result, Exception): | |
| with pytest.raises(expected_result) as exc_info: | |
| Client._parse_timedelta(value, base_time) | |
| assert str(exc_info.value) == str(expected_result()) | |
| else: | |
| result = Client._parse_timedelta(value, base_time) | |
| assert result == expected_result |
| if expected_name is None: | ||
| with pytest.raises(ValidationError): | ||
| Client._create_model_with_expiry(None, Model, response_json, result_expiry) | ||
| Client._create_model_with_expiry(None, Model, response_json, result_expiry, shared_expiry) | ||
| else: | ||
| instance = Client._create_model_with_expiry( | ||
| None, Model, response_json, result_expiry | ||
| None, Model, response_json, result_expiry, shared_expiry | ||
| ) | ||
|
|
||
| # Assert | ||
| assert instance.name == expected_name | ||
| assert instance.age == expected_age | ||
| assert instance.content_expires == expected_expiry | ||
| assert instance.shared_expires == expected_shared_expiry |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if cache_control_header is not None: | ||
| response.headers = {"cache-control": cache_control_header} | ||
| else: | ||
| response.headers = {} |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if isinstance(response_json, dict): | ||
| mock_create_model_with_expiry.assert_called_with( | ||
| Model, response_json, result_expiry | ||
| Model, response_json, result_expiry, shared_expiry | ||
| ) | ||
| else: | ||
| for item in response_json: | ||
| mock_create_model_with_expiry.assert_any_call(Model, item, result_expiry) | ||
| mock_create_model_with_expiry.assert_any_call(Model, item, result_expiry, shared_expiry) |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| for item in response_json: | ||
| mock_create_model_with_expiry.assert_any_call(Model, item, result_expiry) | ||
| mock_create_model_with_expiry.assert_any_call(Model, item, result_expiry, shared_expiry) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| result = self._create_model_instance(Model, data, result_expiry, shared_expiry) | ||
|
|
||
| return result |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| result = self._create_model_instance(Model, data, result_expiry, shared_expiry) | |
| return result | |
| return self._create_model_instance(Model, data, result_expiry, shared_expiry) |
Using 'Optional[datetime]' instead of 'datetime | None' would be more consistent with the rest of the codebase. Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
closes #50
Summary by Sourcery
This pull request adds support for handling both 'max-age' and 's-maxage' headers in cache control, introduces a new 'shared_expires' attribute to various models, and updates the test suite to cover these changes.