Improve add-on submission error reporting #42
Conversation
WalkthroughThe pull request refines error handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Validator
Caller->>Validator: Call _createDictMatchingJsonSchema(manifest)
alt Manifest Version Parsing Error
Validator-->>Caller: Raise ValueError("Manifest version invalid ...")
else Missing Required Key
Validator-->>Caller: Raise KeyError("Manifest missing required key 'keyName'")
else Successful Processing
Validator-->>Caller: Return processed dictionary
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
_validate/createJson.py (4)
94-95: Improve exception handling by preserving the original exception contextThe direct error raising approach is better than setting
manifest._errors, but you should preserve the original exception context usingraise ... from errsyntax.except ValueError as err: - raise ValueError(f"Manifest version invalid {addonVersionNumber}") + raise ValueError(f"Manifest version invalid {addonVersionNumber}") from errThis preserves the exception chain, making debugging easier while still providing the clear error message to users.
🧰 Tools
🪛 Ruff (0.8.2)
95-95: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
117-118: Improve exception handling by preserving the original exception contextSimilar to the previous case, preserve the KeyError's context for better debugging.
except KeyError as e: - raise KeyError(f"Manifest missing required key '{e.args[0]}'.") + raise KeyError(f"Manifest missing required key '{e.args[0]}'.") from e🧰 Tools
🪛 Ruff (0.8.2)
118-118: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
140-141: Improve exception handling by preserving the original exception contextPreserve the exception context for translation-related errors as well.
except KeyError as e: - raise KeyError(f"Translation for {langCode} missing required key '{e.args[0]}'.") + raise KeyError(f"Translation for {langCode} missing required key '{e.args[0]}'.") from e🧰 Tools
🪛 Ruff (0.8.2)
141-141: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
233-243: Consider updating the main error handling section to maintain consistencyThe main error handling block in the
main()function still checksmanifest.errorsfirst. For consistency with your changes in_createDictMatchingJsonSchema, consider updating this section to prioritize the exception message whenmanifest.errorsis empty.except Exception as e: if manifest.errors: if errorFilePath: with open(errorFilePath, "w") as errorFile: errorFile.write(f"Validation Errors:\n{manifest.errors}") raise ValueError(f"Invalid manifest file: {manifest.errors}") else: if errorFilePath: with open(errorFilePath, "w") as errorFile: - errorFile.write(f"Validation Errors:\n{e}") + # Use the exception message which now contains the detailed error information + errorFile.write(f"Validation Errors:\n{str(e)}") raiseThis change ensures that the detailed error messages you've added in the exception raising are properly forwarded to the error output file.
🧰 Tools
🪛 Ruff (0.8.2)
238-238: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
_validate/createJson.py(4 hunks)requirements.txt(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
_validate/createJson.py
95-95: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
118-118: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
141-141: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (3)
requirements.txt (1)
8-9:❓ Verification inconclusive
Significant dependency updates should be verified for compatibility
The PR updates two dependencies:
- configobj to a specific commit (8be54629ee)
- jsonschema from 3.1 to 4.23.0 (a major version jump)
These updates align with the error handling improvements, but the major version upgrade for jsonschema could potentially introduce breaking changes.
Please verify that these updates are compatible with the rest of the codebase, particularly with how schema validation is performed. Consider running the test suite to ensure no regressions are introduced.
🏁 Script executed:
#!/bin/bash # Check for any compatibility issues with jsonschema 4.23.0 pip install jsonschema==4.23.0 --dry-run # Find any files that import jsonschema to understand potential impact rg "import jsonschema" --type pyLength of output: 937
Verify compatibility for critical dependency updates
The PR updates two dependencies:
- configobj: Updated to a specific commit (
8be54629ee7c26acb5c865b74c76284e80f3aa31), likely incorporating important fixes or improvements.- jsonschema: Upgraded from version 3.1 to 4.23.0—a major version jump that can introduce breaking changes.
The dry-run installation for jsonschema 4.23.0 completed without errors, indicating it’s correctly installed. However, the search for
import jsonschemadid not return any output, which raises some uncertainty regarding its direct usage in the codebase. It’s possible that jsonschema is used indirectly or in a non-obvious manner.I recommend:
- Manually verifying any code using jsonschema—for example, checking how schema validation is performed (possibly in files like
createJson.pyor elsewhere).- Running the full test suite to ensure that the major version upgrade doesn’t introduce regressions or compatibility issues.
_validate/createJson.py (2)
3-3: Copyright year updated appropriatelyThe copyright year has been extended to include 2025, which is a good practice for maintaining up-to-date copyright notices.
90-143: Approve improved error handling approachThe changes to directly raise exceptions with meaningful messages rather than setting
manifest._errorsis a significant improvement to error reporting. This ensures that add-on submitters receive clear, specific error messages that help them understand and fix issues with their submissions.This implementation aligns perfectly with the PR objective to "ensure that the relevant error details from
manifest._errorsare communicated to the submitter, thereby improving the clarity and usefulness of the feedback provided during the submission process."🧰 Tools
🪛 Ruff (0.8.2)
95-95: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
118-118: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
131-131: Loop control variable
manifestoverrides iterable it iterates(B020)
141-141: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
|
I agree with the bot's suggestion to use |
When exceptions are raised in submission, the exception itself is reported to the submitter, not the
manifest._errorscontents.This creates a less helpful comment e.g.:
nvaccess/addon-datastore#5198 (comment)
Tested by running createJson.py locally
Also updated dependencies
Summary by CodeRabbit
configobjandjsonschema.