-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pyright #56
Add pyright #56
Conversation
WalkthroughThis update enhances the project's linting and static type checking capabilities by integrating Pyright into both the GitHub Actions workflow and the pre-commit configuration. Additionally, the Changes
Tip New Features and ImprovementsReview SettingsIntroduced new personality profiles for code reviews. Users can now select between "Chill" and "Assertive" review tones to tailor feedback styles according to their preferences. The "Assertive" profile posts more comments and nitpicks the code more aggressively, while the "Chill" profile is more relaxed and posts fewer comments. AST-based InstructionsCodeRabbit offers customizing reviews based on the Abstract Syntax Tree (AST) pattern matching. Read more about AST-based instructions in the documentation. Community-driven AST-based RulesWe are kicking off a community-driven initiative to create and share AST-based rules. Users can now contribute their AST-based rules to detect security vulnerabilities, code smells, and anti-patterns. Please see the ast-grep-essentials repository for more information. New Static Analysis ToolsWe are continually expanding our support for static analysis tools. We have added support for Tone SettingsUsers can now customize CodeRabbit to review code in the style of their favorite characters or personalities. Here are some of our favorite examples:
Revamped Settings PageWe have redesigned the settings page for a more intuitive layout, enabling users to find and adjust settings quickly. This change was long overdue; it not only improves the user experience but also allows our development team to add more settings in the future with ease. Going forward, the changes to Miscellaneous
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
===========================================
+ Coverage 26.09% 37.91% +11.82%
===========================================
Files 3 3
Lines 479 480 +1
===========================================
+ Hits 125 182 +57
+ Misses 354 298 -56 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pyproject.toml
is excluded by!**/*.toml
Files selected for processing (4)
- .github/workflows/pylint.yml (2 hunks)
- .gitignore (1 hunks)
- .pre-commit-config.yaml (1 hunks)
- agility/analysis.py (1 hunks)
Files skipped from review due to trivial changes (2)
- .gitignore
- .pre-commit-config.yaml
Additional Context Used
GitHub Check Runs (1)
codecov/patch failure (1)
agility/analysis.py: [warning] 714-714: agility/analysis.py#L714
Added line #L714 was not covered by tests
Additional comments not posted (2)
.github/workflows/pylint.yml (2)
34-35
: Addition of thepyright
step is a good enhancement for static type checking.
24-24
: Ensure that thepyright
tool is compatible with the Python versions specified in the matrix.
else: | ||
NearestNeighborFinder = None # noqa: N806 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of NearestNeighborFinder = None
when nearest_n
is not provided is a logical change for handling cases where no nearest neighbors are specified. However, this line is not covered by tests.
Would you like me to help by adding unit tests for this scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ab5424, here's a unit test for the scenario where nearest_n
is not provided in the expand_to_non_selected
method. This test checks that NearestNeighborFinder
is set to None
when nearest_n
is None
.
import unittest
from unittest.mock import patch
from agility.analysis import GBStructure
class TestGBStructure(unittest.TestCase):
def test_expand_to_non_selected_without_nearest_n(self):
# Create an instance of GBStructure
gb_structure = GBStructure(backend='ovito', filename='testfile')
# Mock the data attribute to prevent actual file reading
gb_structure.data = unittest.mock.MagicMock()
# Patch the get_finder function to return a mock finder
with patch('agility.analysis.get_finder') as mock_get_finder:
# Set up the mock to return None for NearestNeighborFinder
mock_get_finder.return_value = None
# Call the method with nearest_n as None
result = gb_structure.expand_to_non_selected(nearest_n=None)
# Assert that NearestNeighborFinder was set to None
self.assertIsNone(result)
if __name__ == '__main__':
unittest.main()
This test uses Python's unittest
framework and includes mocking to isolate the test scenario. Let me know if you need further modifications or additional tests!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .pre-commit-config.yaml (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- .pre-commit-config.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
tests/analysis/test_analysis.py (1)
8-8
: Ensure consistent import order.It's a good practice to keep imports grouped and ordered alphabetically. Consider moving
import numpy as np
aboveimport pytest
.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/analysis/test_analysis.py (2 hunks)
Additional comments not posted (3)
tests/analysis/test_analysis.py (3)
56-84
: Review of the new test classTestGBStructureOxide
.The new test class
TestGBStructureOxide
is well-structured and follows the established patterns in the existing test suite. The use ofpytest.mark.filterwarnings
to ignore specific warnings is appropriate and helps focus the tests on the intended behaviours.
59-63
: Proper setup for test instances.The
setUp
method inTestGBStructureOxide
correctly initialises the test instance and includes a safety check to ensure the instance is properly created. This is a good practice in unit testing.
65-84
: Thorough testing of expansion logic.The
test_expand_to_non_selected_
method inTestGBStructureOxide
provides a comprehensive test of the expansion logic to non-selected particles. The assertions are well-placed to verify the correctness of the functionality.
No description provided.