chore: replace darglint with pydoclint
#749
Conversation
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: pypi/darglint@1.8.1, pypi/flake8-annotations@2.7.0, pypi/flake8@3.9.2, pypi/mccabe@0.6.1, pypi/poethepoet@0.19.0, pypi/pycodestyle@2.7.0, pypi/pyflakes@2.3.1 |
|
@coderabbitai full review |
Actions performedFull review triggered. |
WalkthroughThe changes in this pull request involve the removal of the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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: 1
Outside diff range and nitpick comments (3)
.flake8 (1)
14-17: Temporary workaround looks good, but track the TODO.Allowing docstrings in
__init__functions as a temporary measure for easier migration fromdarglintis a pragmatic approach. However, please ensure that the TODO comment is tracked and addressed in the future to maintain consistency with standard docstring conventions.xrpl/core/binarycodec/types/st_object.py (2)
61-63: Linting warning suppressed, but consider addressing the underlying issue.The addition of the
# noqa: ANN401comment suppresses the linting warning related to the return type annotation of the_str_to_enumfunction. While this allows the code to pass the linting checks, it's important to consider addressing the underlying issue to improve code quality and maintainability.Consider updating the return type annotation to accurately reflect the possible return values of the function. This will not only resolve the linting warning but also provide better type information for users of the function.
73-74: Linting warning suppressed, but consider addressing the underlying issue.The addition of the
# noqa: ANN401comment suppresses the linting warning related to the return type annotation of the_enum_to_strfunction. While this allows the code to pass the linting checks, it's important to consider addressing the underlying issue to improve code quality and maintainability.Consider updating the return type annotation to accurately reflect the possible return values of the function. This will not only resolve the linting warning but also provide better type information for users of the function.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
Files selected for processing (16)
- .darglint (0 hunks)
- .flake8 (3 hunks)
- .github/workflows/unit_test.yml (1 hunks)
- .pre-commit-config.yaml (1 hunks)
- CONTRIBUTING.md (1 hunks)
- pyproject.toml (3 hunks)
- tests/unit/core/binarycodec/types/test_serialized_type.py (1 hunks)
- xrpl/core/addresscodec/codec.py (1 hunks)
- xrpl/core/binarycodec/types/serialized_type.py (2 hunks)
- xrpl/core/binarycodec/types/st_object.py (2 hunks)
- xrpl/models/base_model.py (3 hunks)
- xrpl/models/nested_model.py (1 hunks)
- xrpl/models/requests/generic_request.py (1 hunks)
- xrpl/models/response.py (2 hunks)
- xrpl/transaction/main.py (1 hunks)
- xrpl/utils/xrp_conversions.py (3 hunks)
Files not reviewed due to no reviewable changes (1)
- .darglint
Additional comments not posted (26)
.pre-commit-config.yaml (1)
24-24: LGTM! The change aligns with the PR objective of replacingdarglintwithpydoclint.The removal of the
--darglint-ignore-regexargument from theflake8command is consistent with the goal of replacingdarglintwithpydoclintfor docstring linting. This change may result in additional warnings being reported during the pre-commit checks, as the specified darglint warnings will no longer be ignored.Ensure that all relevant docstrings have been updated to comply with the new linting rules enforced by
pydoclint. The PR objectives indicate that all linter issues arising from the upgrades have been addressed, which suggests that the necessary updates have been made..flake8 (3)
21-21: LGTM!Updating the
per-file-ignoressetting for test files to useDOCinstead ofDARaligns with the PR objective of replacingdarglintwithpydoclint. This ensures that test files are linted using the new docstring style rules.
33-33: Comment adds clarity.Adding a comment to clarify that
DOCenables docstring style linting viapydoclintis helpful for developers. It provides a clear indication of the tool being used for this purpose.
43-43: LGTM!Updating the
selectoption to includeDOCand removeDARaligns with the PR objective of replacingdarglintwithpydoclint. This change ensures that the linter applies the new docstring style rules consistently across the codebase.pyproject.toml (6)
33-33: Clarify the reasoning behind the Python version bump.The Python version requirement change from
^3.8to^3.8.1looks good and is unlikely to cause any compatibility issues. However, could you please provide some context on why the minimum version was bumped specifically to 3.8.1? This will help maintain a clear record of the rationale behind the change.
44-44: Verify the impact of the flake8 and flake8-annotations upgrades.The upgrades to
flake8(to^7.0.0) andflake8-annotations(to3.1.1) look good, considering that the PR objectives state all linter issues arising from these upgrades have been addressed.However, given that the
flake8upgrade is a major version bump, it would be prudent to double-check that the new linting rules do not flag any unintended issues in the codebase. Please verify that the linter passes cleanly after these upgrades.Also applies to: 51-51
53-53: Monitor pydoclint's stability and verify its impact on the codebase.The migration from
darglinttopydoclintlooks good as it aligns with the PR objective of replacing the outdateddarglintwith a more actively maintained and faster alternative. The reduction in linting time from 23 seconds to 9 seconds is a significant improvement.However, please note that
pydoclintis still in an early version (^0.5.7), which may indicate potential instability or future breaking changes. It would be wise to monitorpydoclint's releases and stability going forward.Additionally, please verify that the new
pydoclintrules do not flag any unintended documentation issues in the codebase.
60-60: Provide more context on the poethepoet upgrade.The upgrade of
poethepoetfrom^0.19.0to^0.28.0looks good as it is a minor version bump, which generally indicates the addition of new features and bug fixes without introducing breaking changes.However, could you please provide more context on the motivation behind this upgrade? Are there any specific features or fixes from the newer
poethepoetversions that are being leveraged in this project? This information will help maintain a clear record of the reasoning behind the upgrade.
83-83: LGTM!The addition of the
linttask under[tool.poe.tasks]is a great improvement to the project's linting capabilities. Runningflake8on thexrpl,tests, andsnippetsdirectories will help maintain code quality and consistency by enforcing coding standards and identifying potential issues early in the development process. This task will be a valuable addition to the development workflow and CI/CD pipeline.
Line range hint
1-90: Great work on improving the project's dependencies and linting capabilities!The changes made in this PR, including the dependency upgrades, migration to
pydoclint, and addition of thelinttask, align well with the stated objectives of keeping the project up-to-date, improving performance, and enhancing code quality. These changes collectively contribute to a better development workflow, increased maintainability, and more consistent code.The migration to
pydoclintand the addition of thelinttask are particularly noteworthy, as they can significantly reduce linting time and catch potential issues early in the development process.Overall, the changes look good, subject to addressing any outstanding questions or concerns raised in the previous review comments. Keep up the great work!
tests/unit/core/binarycodec/types/test_serialized_type.py (1)
51-51: LGTM!The change from
type(fixture.test_json) == dicttoisinstance(fixture.test_json, dict)is a good improvement. It enhances the robustness of the type checking by allowing for subclassing, which is more Pythonic and flexible.xrpl/models/nested_model.py (1)
24-24: Verify if suppressing the linting warning is justified.The change of adding the
# noqa: ANN401comment directive to suppress the specific linting warning related to type annotations does not impact the functionality or correctness of theis_dict_of_modelmethod.However, please ensure that suppressing the
ANN401warning is justified and aligns with the project's linting guidelines. If the suppression is deemed necessary, consider adding a comment explaining the reason for suppressing the warning to maintain code clarity.To verify the suppression, run the following script:
Verification successful
ANN401 suppression is justified and consistent with project practices.
The addition of
# noqa: ANN401to suppress the linting warning for theAnytype annotation in theis_dict_of_modelmethod is justified and aligns with the project's coding practices. Here's why:
- The suppression is used consistently throughout the codebase for functions with
Anytype annotations, indicating a deliberate coding style choice.- The use of
Anytype and its suppression appears necessary due to the nature of the XRPL library, which likely needs to handle various data types flexibly.- The suppression is not overused, appearing only in specific scenarios where the
Anytype is required.- The project demonstrates a judicious use of linting suppressions overall, suggesting that this particular suppression is part of a well-considered approach to code quality.
The change is approved, as it maintains consistency with the existing codebase practices and does not impact the functionality of the method.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if suppressing the ANN401 linting warning is justified. # Test: Search for the suppression comment in the codebase. Expect: Only occurrences in justified cases. rg --type python $'# noqa: ANN401'Length of output: 66
Script:
#!/bin/bash # Description: Verify the usage of linting suppressions in Python files # Search for "# noqa: ANN401" in Python files echo "Occurrences of '# noqa: ANN401':" fd -e py -x rg -n "# noqa: ANN401" {} echo -e "\nAll occurrences of 'noqa' in Python files:" fd -e py -x rg -n "# noqa:" {} echo -e "\nPython files containing 'noqa':" fd -e py -x rg -l "# noqa:" {} | sort | uniqLength of output: 5183
xrpl/core/binarycodec/types/serialized_type.py (3)
28-28: Suppressing ANN401 linting error for flexibility.The addition of the
# noqa: ANN401comment directive to suppress the linting error for theAnytype annotation on thelength_hintparameter is a reasonable decision. It allows subclasses to define their own type requirements forlength_hint, providing flexibility in the implementation.While using
Anyis generally discouraged, in this case, it serves a purpose to accommodate different use cases in subclasses. The suppression of the linting error is justified given the context and the need for adaptability.
34-34: Suppressing D102 and ANN401 linting errors for flexibility and brevity.The addition of the
# noqa: D102 ANN401comment directive to suppress the linting errors for the missing docstring and theAnytype annotation on thevalueparameter is a reasonable decision in this context. It allows thefrom_valuemethod to accept different types of values, providing flexibility in the implementation.While missing docstrings and using
Anyare generally discouraged, in this case, the developer has made a conscious choice to prioritize flexibility and brevity. The suppression of the linting errors is justified given the specific requirements and design decisions of the codebase.
57-57: Suppressing ANN401 linting error for flexibility in JSON representation.The addition of the
# noqa: ANN401comment directive to suppress the linting error for theAnytype annotation on the return value of theto_jsonmethod is a reasonable decision. It allows the method to return different types of JSON representations, providing flexibility in the implementation.While using
Anyis generally discouraged, in this case, it serves a purpose to accommodate different JSON structures that may be returned by subclasses or overridden implementations. The suppression of the linting error is justified given the context and the need for adaptability in the JSON representation.xrpl/models/requests/generic_request.py (1)
31-31: LGTM!The addition of the
# noqa: ANN401directive to suppress the linting warning related to the lack of type annotation for thekwargsparameter is appropriate in this case. The directive indicates that the developer has intentionally chosen not to annotatekwargswith a specific type, as it is meant to accept any keyword arguments.The change addresses the linting feedback without altering the functionality of the
__init__method, which remains unchanged. This stylistic adjustment improves code quality while maintaining the intended behavior of the method.xrpl/utils/xrp_conversions.py (2)
39-39: LGTM!The change from
type(xrp) == strtoisinstance(xrp, str)is an improvement as it allows for subclassing and is considered a best practice for type checking in Python. The rest of the function logic remains unchanged, and the overall functionality is not affected.
87-87: LGTM!The change from
type(drops) != strtonot isinstance(drops, str)is an improvement as it allows for subclassing and is considered a best practice for type checking in Python. The rest of the function logic remains unchanged, and the overall functionality is not affected.xrpl/transaction/main.py (1)
2-2: LGTM!Adding a blank line after the module docstring is consistent with the PEP 8 style guide and enhances code readability. Good catch!
xrpl/models/response.py (2)
95-95: LGTM!The addition of the
# noqa: ANN401comment suppresses the linting ruleANN401for this function. This rule is likely related to missing type annotations. Since this is an internal function and the types can be inferred from the context, suppressing this specific rule is acceptable.
114-114: LGTM!Similar to the
_do_contains_partial_paymentfunction, the addition of the# noqa: ANN401comment suppresses the linting ruleANN401for this function. This rule is likely related to missing type annotations. Since this is an internal function and the types can be inferred from the context, suppressing this specific rule is acceptable.xrpl/core/addresscodec/codec.py (1)
43-44: LGTM!Splitting the long error message into two lines improves readability without altering the semantic content. The error message still conveys the same information about the mismatch between the length of the provided bytestring and the expected length.
CONTRIBUTING.md (1)
69-69: LGTM!The change from directly running
flake8to usingpoetry run poe lintis a good improvement. It abstracts the linting process and makes it easier for contributors to run the linter with the correct arguments and configurations. This reduces the chances of contributors running the linter with incorrect settings and ensures consistency across all contributions.xrpl/models/base_model.py (3)
88-88: LGTM!The addition of the
# noqa: ANN401comment is used to suppress a specific type annotation related linting warning (likely from theflake8-annotationsplugin). This is an acceptable way to silence the linter when the type hint is complex or the linter is raising a false positive. The method functionality remains unaffected.
153-153: LGTM!As explained in the previous comment, the
# noqa: ANN401comment is an acceptable way to suppress a specific type annotation related linting warning when the type hint is complex or the linter is raising a false positive. The method functionality remains unaffected.
324-324: LGTM!As explained in the previous comments, the
# noqa: ANN401comment is an acceptable way to suppress a specific type annotation related linting warning when the type hint is complex or the linter is raising a false positive. The method functionality remains unaffected.
| @@ -49,7 +49,7 @@ jobs: | |||
|
|
|||
| - name: Lint | |||
| run: | | |||
| poetry run flake8 xrpl tests snippets --darglint-ignore-regex="^_(.*)" | |||
| poetry run poe lint | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Review and adjust the new linting configuration
The change to poetry run poe lint does include linting checks, but the configuration has changed:
- The linting now uses the Google docstring convention instead of NumPy.
- Some docstring-related rules (D205, D212, D415) are now ignored.
- Docstring linting is disabled for test files.
Please review the new configuration in the .flake8 file to ensure it meets the project's needs, especially regarding docstring conventions and ignored rules. You may need to update the codebase to comply with the Google docstring convention if it was previously using NumPy.
Analysis chain
Verify if the specific regex option for ignoring certain docstring checks is still required.
The change to use poetry run poe lint improves the organization of the linting process by encapsulating multiple linting tools or configurations under the poe command.
However, please verify if the specific regex option --docstring-convention=numpy --ignore=D100,D101,D102,D103,D104,D105,D106,D107 for ignoring certain docstring checks is still required and included in the new poe lint command. If not, it might lead to new linting errors related to docstrings.
To verify if the specific regex option is still required, run the following script:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the specific regex option for ignoring certain docstring checks is still required.
# Test 1: Search for the specific regex option in the `poe` configuration file.
# Expect: The specific regex option is present in the `poe` configuration file.
rg --type toml $'--docstring-convention=numpy --ignore=D100,D101,D102,D103,D104,D105,D106,D107' pyproject.toml
# Test 2: Run the `poe lint` command and check for docstring-related linting errors.
# Expect: No docstring-related linting errors are reported.
poetry run poe lint
Length of output: 633
Script:
#!/bin/bash
# Description: Verify if the specific regex option for ignoring certain docstring checks is still required.
# Test 1: Search for flake8 configuration in pyproject.toml
echo "Searching for flake8 configuration in pyproject.toml:"
cat pyproject.toml | sed -n '/\[tool\.flake8\]/,/^\[/p'
# Test 2: Search for poe tasks related to linting in pyproject.toml
echo "Searching for poe tasks related to linting in pyproject.toml:"
cat pyproject.toml | sed -n '/\[tool\.poe\.tasks\]/,/^\[/p' | grep -i 'lint'
# Test 3: Check if there's a separate .flake8 configuration file
echo "Checking for .flake8 configuration file:"
if [ -f ".flake8" ]; then
cat .flake8
else
echo ".flake8 file not found"
fi
# Test 4: List all Python files in the project
echo "Listing Python files in the project:"
find . -name "*.py" | head -n 5
Length of output: 2245
There was a problem hiding this comment.
The linting always used the Google docstring convention. The settings used are still in the .flake8 file, the only one not included is the darglint-specific error (which wasn't supported by the .flake8 file).
There was a problem hiding this comment.
@mvadari, thank you for the clarification! It's helpful to know that the linting has always used the Google docstring convention and that the settings are still in the .flake8 file, except for the darglint-specific error which wasn't supported there. I appreciate your explanation.
(^.^)?
Learnings added
Learnt from: mvadari
PR: XRPLF/xrpl-py#749
File: .github/workflows/unit_test.yml:52-52
Timestamp: 2024-09-19T19:59:13.121Z
Learning: - The project has always used the Google docstring convention for linting.
- The linting settings are in the `.flake8` file.
- The darglint-specific error wasn't supported in `.flake8`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
High Level Overview of Change
darglint, the docstring linter we use, hasn't been updated in 3 years.pydoclintis kept up to date and is also much faster (linting took 23 seconds previously and now only takes 9).This PR also creates a
lintcommand inpoe, upgradesflake8andpoethepoetto the latest versions, and fixes all linter issues that arose from the upgrade.Context of Change
Fixes #730
Type of Change
Did you update CHANGELOG.md?
Test Plan
The linter still passes.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores