MPT-17825: resolve WPS111 naming violations#135
Merged
Conversation
svazquezco
force-pushed
the
codex/MPT-17825-adopt-wemake-rules-WPS111
branch
from
48979e7 to
f528d46
Compare
📝 WalkthroughWalkthroughRefactor-only changes renaming local variables for clarity (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/core/file_discovery.py (1)
26-26: Consider avoiding variable shadowing for clarity.The lambda parameter
file_pathshadows the loop variable from line 17, which may reduce readability even though the loop has completed.♻️ Alternative approaches to avoid shadowing
Option 1: Use a different parameter name
- return list(filter(lambda file_path: file_path.endswith(".xlsx"), file_paths)) + return list(filter(lambda path: path.endswith(".xlsx"), file_paths))Option 2: Use a list comprehension (more Pythonic)
- return list(filter(lambda file_path: file_path.endswith(".xlsx"), file_paths)) + return [path for path in file_paths if path.endswith(".xlsx")]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/core/file_discovery.py` at line 26, The return statement in file_discovery.py uses a lambda with parameter name file_path that shadows the outer variable of the same name; to fix, avoid shadowing by either renaming the lambda parameter (e.g., to p) or, preferably, replace the filter+lambda with a list comprehension (e.g., [p for p in file_paths if p.endswith(".xlsx")]) inside the same function to make intent clearer and improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/core/price_lists/services/price_list_service.py`:
- Around line 18-20: In the PriceListService.create method replace the broad
"except Exception as error" with "except MPTAPIError as error" so it matches
retrieve/retrieve_from_mpt/update; ensure MPTAPIError is imported where
PriceListService (or the create method) is defined, keep the existing
self._set_error(str(error)) and the ServiceResult(success=False,
errors=[str(error)], model=None, stats=self.stats) return unchanged.
---
Nitpick comments:
In `@cli/core/file_discovery.py`:
- Line 26: The return statement in file_discovery.py uses a lambda with
parameter name file_path that shadows the outer variable of the same name; to
fix, avoid shadowing by either renaming the lambda parameter (e.g., to p) or,
preferably, replace the filter+lambda with a list comprehension (e.g., [p for p
in file_paths if p.endswith(".xlsx")]) inside the same function to make intent
clearer and improve readability.
f528d46 to
d61ba59
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cli/core/errors.py (2)
73-74: Consider adding exception chaining here as well.Same as above - using
raise ... from errorpreserves the exception chain for better debugging.Suggested improvement
- raise MPTAPIError(str(error), error.body) + raise MPTAPIError(str(error), error.body) from error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/core/errors.py` around lines 73 - 74, The except block catching APIException should preserve exception chaining; update the raise of MPTAPIError in the except APIException as error handler to re-raise using exception chaining (i.e., raise MPTAPIError(str(error), error.body) from error) so the original APIException is preserved for debugging; locate the except APIException handler and modify the raise to include "from error" referencing APIException and MPTAPIError.
44-59: Variable renaming looks good. Consider adding exception chaining.The rename from
etoerrorand the inner loop variable change fromerrortoerror_details(to avoid shadowing) are correct.The static analyzer suggests using
raise ... from errorto preserve the exception chain, which aids debugging by keeping the original traceback.Suggested improvement
- raise MPTAPIError(str(error), msg) + raise MPTAPIError(str(error), msg) from error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/core/errors.py` around lines 44 - 59, Update the exception raise to preserve the original traceback by using exception chaining: in the RequestException handler where you currently call raise MPTAPIError(str(error), msg), change it to raise MPTAPIError(str(error), msg) from error so the original RequestException (error) is attached; this involves the try/except block catching RequestException and the raise of MPTAPIError.cli/core/products/services/product_service.py (1)
28-28: Narrow exception type from bareExceptiontoMPTAPIErrorfor consistency.Ruff flags these as
BLE001: Do not catch blind exception: Exception. All three API methods called here—self.api.post(),self.api.exists(), andself.api.get()—are decorated with@wrap_http_error, which converts HTTP errors toMPTAPIError. The same file already usesMPTAPIErrorcorrectly on lines 35 and 134, so changing lines 28, 72, and 82 to catchMPTAPIErroraligns with existing patterns and improves error clarity.This appears to be pre-existing behavior not introduced by this PR, so it can be addressed separately if desired.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/core/products/services/product_service.py` at line 28, Change the three broad except blocks that currently catch bare Exception around the API calls to catch MPTAPIError instead: update the handlers that wrap calls to self.api.post(), self.api.exists(), and self.api.get() so they except MPTAPIError (the same error type already used elsewhere in ProductService) and keep the existing error handling logic unchanged; this narrows the exception type to the HTTP-wrapped error produced by the `@wrap_http_error` decorator and aligns with existing usage of MPTAPIError in the class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cli/core/test_errors.py`:
- Line 1: Replace the direct use of unittest.mock.Mock with the pytest-mock
fixture: remove the "from unittest.mock import Mock" import and update the
affected test functions in tests/cli/core/test_errors.py (including the ones
around lines 26 and 38) to accept the pytest "mocker" fixture and use
mocker.Mock or mocker.patch in place of Mock; ensure any calls constructing
Mock() are replaced with mocker.Mock() and any patching uses mocker.patch so the
tests follow the pytest-mock guideline.
---
Duplicate comments:
In `@cli/core/price_lists/services/price_list_service.py`:
- Around line 18-20: Replace the broad except Exception in the method that
currently does "except Exception as error" with a specific "except MPTAPIError
as error" to match the other handlers (see retrieve, retrieve_from_mpt, update)
so you only catch the expected MPT API failures; keep the existing error
handling body (self._set_error(str(error)) and returning ServiceResult(...))
unchanged.
---
Nitpick comments:
In `@cli/core/errors.py`:
- Around line 73-74: The except block catching APIException should preserve
exception chaining; update the raise of MPTAPIError in the except APIException
as error handler to re-raise using exception chaining (i.e., raise
MPTAPIError(str(error), error.body) from error) so the original APIException is
preserved for debugging; locate the except APIException handler and modify the
raise to include "from error" referencing APIException and MPTAPIError.
- Around line 44-59: Update the exception raise to preserve the original
traceback by using exception chaining: in the RequestException handler where you
currently call raise MPTAPIError(str(error), msg), change it to raise
MPTAPIError(str(error), msg) from error so the original RequestException (error)
is attached; this involves the try/except block catching RequestException and
the raise of MPTAPIError.
In `@cli/core/products/services/product_service.py`:
- Line 28: Change the three broad except blocks that currently catch bare
Exception around the API calls to catch MPTAPIError instead: update the handlers
that wrap calls to self.api.post(), self.api.exists(), and self.api.get() so
they except MPTAPIError (the same error type already used elsewhere in
ProductService) and keep the existing error handling logic unchanged; this
narrows the exception type to the HTTP-wrapped error produced by the
`@wrap_http_error` decorator and aligns with existing usage of MPTAPIError in the
class.
svazquezco
force-pushed
the
codex/MPT-17825-adopt-wemake-rules-WPS111
branch
from
d61ba59 to
f2edab6
Compare
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cli/plugins/audit_plugin/api.py (1)
14-14: Consider narrowing exception types (out of scope for this PR).Ruff flags
BLE001on the blindexcept Exceptionclauses. This is a pre-existing pattern, not introduced by your changes. As a future improvement, consider catching more specific exceptions (e.g.,requests.RequestExceptionor the client's specific error type) to avoid masking unexpected errors.Also applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/plugins/audit_plugin/api.py` at line 14, Replace the broad "except Exception as error" handlers in the audit plugin API (the two occurrences of "except Exception as error") with narrower exception types appropriate to the operations being performed (e.g., requests.RequestException for HTTP calls, the specific client's error class for client calls, or ValueError/IOError where applicable), and add the necessary imports; if multiple error types must be handled, use separate except blocks or a tuple of exception classes to avoid masking unexpected exceptions while preserving existing error-logging behavior.tests/cli/core/price_lists/services/test_item_service.py (1)
129-147: Assert the update call to lock the failure path.
Without asserting the update invocation, this test could pass even if the update path is skipped in future changes.Proposed tweak
- mocker.patch.object( + update_mock = mocker.patch.object( service_context.api, "update", side_effect=MPTAPIError("API Error", "Error updating item"), ) @@ - file_handler_mock.assert_called() + update_mock.assert_called_once() + file_handler_mock.assert_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli/core/price_lists/services/test_item_service.py` around lines 129 - 147, The test test_update_item_api_update_error should explicitly assert that the API update path was invoked; modify the test to assert that service_context.api.update was called (e.g., using mocker to get the mock and calling assert_called_once or assert_called_with) after calling ItemService(service_context).update() so the failure path is locked in—reference the mocked service_context.api.update and the ItemService.update invocation and keep the existing assertions on result.success, result.errors, and file_manager.write_error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cli/plugins/audit_plugin/api.py`:
- Line 14: Replace the broad "except Exception as error" handlers in the audit
plugin API (the two occurrences of "except Exception as error") with narrower
exception types appropriate to the operations being performed (e.g.,
requests.RequestException for HTTP calls, the specific client's error class for
client calls, or ValueError/IOError where applicable), and add the necessary
imports; if multiple error types must be handled, use separate except blocks or
a tuple of exception classes to avoid masking unexpected exceptions while
preserving existing error-logging behavior.
In `@tests/cli/core/price_lists/services/test_item_service.py`:
- Around line 129-147: The test test_update_item_api_update_error should
explicitly assert that the API update path was invoked; modify the test to
assert that service_context.api.update was called (e.g., using mocker to get the
mock and calling assert_called_once or assert_called_with) after calling
ItemService(service_context).update() so the failure path is locked in—reference
the mocked service_context.api.update and the ItemService.update invocation and
keep the existing assertions on result.success, result.errors, and
file_manager.write_error.
d3rky
approved these changes
Feb 18, 2026
This was referenced Feb 18, 2026
This was referenced Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Codex-generated PR — Please review carefully.
Summary
WPS111from flake8per-file-ignoresinpyproject.tomlWPS111(exceptions, iterators, lambdas, file handles)Validation
make checkmake testCloses MPT-17825