fix: steps is now use as a local variable accumulator instead of gloabal. #237
Conversation
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| def unit_conversion_requests( | ||
| number_of_requests: int, request_unit: str, steps: list[str] | ||
| ) -> int: |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note
Copilot Autofix
AI 24 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
|
|
||
|
|
||
| def unit_conversion_memory(memory: float, memory_unit: str) -> float: | ||
| def unit_conversion_memory(memory: float, memory_unit: str, steps: list[str]) -> float: |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 24 days ago
To fix the issue, we need to ensure that unit_conversion_memory never falls off the end of the function and implicitly returns None. The best way to do this is to add an explicit return at the end of the function, preferably return None, since if execution ever reaches the end without hitting one of the match cases, it most likely denotes an error or unexpected value for memory_unit. No changes in imports or other definitions are required, just a single line addition at the end of the function at line 106 in aws-lambda-calculator/src/aws_lambda_calculator/calculator.py.
| @@ -103,8 +103,8 @@ | ||
| return memory | ||
| case _: | ||
| raise ValueError(f"Unknown memory unit: {memory_unit}") | ||
| return None | ||
|
|
||
|
|
||
| def unit_conversion_ephemeral_storage( | ||
| ephemeral_storage_mb: float, storage_unit: str, steps: list[str] | ||
| ) -> float: |
There was a problem hiding this comment.
Pull Request Overview
This PR removes the module-level global steps and refactors it into a passed-in accumulator list, making the computation trace explicit and locally scoped.
- Add steps: list[str] parameter to conversion and pricing functions; propagate through calculate and tests.
- Remove global steps; initialize a local steps list in calculate.
- Update tests to pass a local steps list into affected functions.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| aws-lambda-calculator/src/aws_lambda_calculator/calculator.py | Removes global steps, adds steps parameter across functions, and wires it through calculate. |
| aws-lambda-calculator/tests/test_calculator_coverage.py | Updates tests to pass a local steps list to affected APIs. |
Comments suppressed due to low confidence (2)
aws-lambda-calculator/src/aws_lambda_calculator/calculator.py:141
- The docstring is missing documentation for the new steps parameter; please add a brief description (e.g., 'steps: list collecting calculation trace lines').
def calculate_tiered_cost(
total_compute_gb_sec: float,
tier_cost_factor: dict[str, float],
overflow_rate: float,
steps: list[str],
) -> float:
"""
total_compute_gb_sec: total usage in GB‑seconds
tier_cost_factor: maps breakpoint (as string) → rate
overflow_rate: per‑GB‑sec rate for usage beyond the highest breakpoint
aws-lambda-calculator/src/aws_lambda_calculator/calculator.py:141
- Docstring parameter list for calculate_tiered_cost omits steps; include an entry (e.g., 'steps: accumulator for detailed trace messages') to keep documentation consistent with the signature.
"""
total_compute_gb_sec: total usage in GB‑seconds
tier_cost_factor: maps breakpoint (as string) → rate
overflow_rate: per‑GB‑sec rate for usage beyond the highest breakpoint
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| def unit_conversion_requests( | ||
| number_of_requests: int, request_unit: str, steps: list[str] | ||
| ) -> int: |
There was a problem hiding this comment.
[nitpick] Adding a required steps parameter is a breaking API change for external callers. Consider making it optional to preserve backward compatibility, e.g., steps: list[str] | None = None and only append when steps is not None; apply the same pattern to unit_conversion_memory and unit_conversion_ephemeral_storage.
|
|
||
|
|
||
| def unit_conversion_memory(memory: float, memory_unit: str) -> float: | ||
| def unit_conversion_memory(memory: float, memory_unit: str, steps: list[str]) -> float: |
There was a problem hiding this comment.
[nitpick] Same breaking change concern: make steps optional (steps: list[str] | None = None) and guard appends to avoid forcing all callers to pass a list.
| ephemeral_storage_mb: float, storage_unit: str, steps: list[str] | ||
| ) -> float: |
There was a problem hiding this comment.
[nitpick] Same breaking change concern: make steps optional and only append when provided, to avoid breaking existing consumers.
| requests_per_month: float, requests_cost_factor: float, steps: list[str] | ||
| ) -> float: |
There was a problem hiding this comment.
[nitpick] Same as above: make steps optional to preserve backward compatibility with existing callers.
| storage_in_gb: float, | ||
| ephemeral_storage_cost_factor: float, | ||
| total_compute_gb_sec: float, | ||
| steps: list[str], | ||
| ) -> float: |
There was a problem hiding this comment.
[nitpick] Same as above: make steps optional to avoid breaking changes; only append when steps is not None.
| steps = [] | ||
| result = unit_conversion_requests(100, "per minute", steps) |
There was a problem hiding this comment.
[nitpick] steps = [] is repeated across many tests; consider a pytest fixture (e.g., a steps() function returning a new list) to reduce duplication and make the intent clearer.
PR Type
Bug fix
Description
Remove global
stepsvariable, convert to local parameterPass
stepslist as parameter to all functions that append to itUpdate all function calls to include
stepsargumentUpdate test cases to initialize and pass
stepsparameterDiagram Walkthrough
File Walkthrough
calculator.py
Convert steps from global to local parameteraws-lambda-calculator/src/aws_lambda_calculator/calculator.py
steps = []declaration at module levelsteps: list[str]parameter tounit_conversion_requests(),unit_conversion_memory(),unit_conversion_ephemeral_storage(),calculate_tiered_cost(),calc_monthly_compute_charges(),calc_monthly_request_charges(), andcalc_monthly_ephemeral_storage_charges()stepsas local variable incalculate()functionstepsargumenttest_calculator_coverage.py
Update tests to pass steps parameteraws-lambda-calculator/tests/test_calculator_coverage.py
steps = []list in each test method before callingfunctions
stepsparameter in testassertions
signatures