-
Notifications
You must be signed in to change notification settings - Fork 24
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
[WIP] Update continuous integration and contributing guide #368
Conversation
WalkthroughThe pull request introduces configuration updates to the Changes
Possibly related PRs
Suggested labels
Poem
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 (
|
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
🧹 Nitpick comments (1)
.pre-commit-config.yaml (1)
Line range hint
15-15
: Address the TODO comment regarding notebook formatting.There's a pending task to fix notebooks so that the
types_or
configuration can be simplified. Would you like assistance in creating a tracking issue for this?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.pre-commit-config.yaml
(1 hunks)
🔇 Additional comments (2)
.pre-commit-config.yaml (2)
1-5
: Well-structured CI configuration section.The new CI configuration provides clear automation settings with descriptive commit messages and a reasonable monthly update schedule.
3-3
: Verify the necessity of skipping pyright checks.Skipping the pyright hook in CI might lead to type-related issues being missed. Could you please clarify why this skip is necessary?
Let's check if there are any recent pyright-related issues:
✅ Verification successful
Skipping pyright checks is consistent with project configuration
The project explicitly configures
typeCheckingMode = "off"
in pyproject.toml, indicating a deliberate choice for gradual typing adoption. The skip in pre-commit aligns with this configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for pyright-related issues in recent commits and PR discussions # Search for pyright-related comments in recent PRs gh pr list --state all --limit 10 --json number,title,body | \ jq '.[] | select(.title | test("pyright"; "i") or .body | test("pyright"; "i"))' # Search for pyright-related commits git log --grep="pyright" --since="1 month ago" --pretty=format:"%h %s"Length of output: 208
Script:
#!/bin/bash # Check for Python files and type-checking configuration echo "=== Python Files ===" fd -e py --type f echo -e "\n=== Pyright Configuration ===" fd -g 'pyrightconfig.json' fd -g 'pyproject.toml' -x cat {} fd -g 'setup.cfg' -x cat {} echo -e "\n=== Type Hints Usage ===" rg "from typing import" -l rg "import typing" -lLength of output: 10859
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #368 +/- ##
========================================
Coverage 78.04% 78.04%
========================================
Files 31 31
Lines 2642 2642
========================================
Hits 2062 2062
Misses 580 580 ☔ 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: 0
🧹 Nitpick comments (5)
CONTRIBUTING.md (4)
71-71
: Fix typographical error in the instruction.There's an extra space before the closing parenthesis.
-Fetch the latest changes from the master branch to keep it up to date (make sure you are on the master branch ). +Fetch the latest changes from the master branch to keep it up to date (make sure you are on the master branch).
Line range hint
82-82
: Add missing comma for better readability.-For a general overview of using pull requests on GitHub look [in the GitHub docs] +For a general overview of using pull requests on GitHub, look [in the GitHub docs]🧰 Tools
🪛 LanguageTool
[style] ~10-~10: Would you like to use the Oxford spelling “summarized”? The spelling ‘summarised’ is also correct.
Context: ...he steps for a new piece of work can be summarised as follows: 1. Push up or create [an i...(OXFORD_SPELLING_Z_NOT_S)
[style] ~61-~61: Consider removing “of” to be more concise
Context: ...are finished with the work, ensure that all of the unit tests pass on your own machine by ...(ALL_OF_THE)
[style] ~78-~78: Consider shortening or rephrasing this to strengthen your wording.
Context: ...uld lead to merge conflicts if you have made changes to the master branch. ## Pull requests F...(MAKE_CHANGES)
96-96
: Fix grammatical error in the comment.-pip install -e ".[dev,docs]" # Should be ran from the root of the repository +pip install -e ".[dev,docs]" # Should be run from the root of the repository
21-43
: Consider adding a note about uv's installation requirements.The guide should mention that
uv
requires Python 3.8+ and pip to be installed. Also, consider adding platform-specific activation commands for Windows users.docs/CONTRIBUTING.md (1)
1-117
: Consider using a symbolic link or documentation include directive.To avoid maintaining identical content in two locations, consider either:
- Using a symbolic link from docs/CONTRIBUTING.md to CONTRIBUTING.md
- Using Sphinx's include directive to include CONTRIBUTING.md in the documentation
This would reduce maintenance overhead and prevent potential inconsistencies.
Example using Sphinx's include directive:
.. include:: ../CONTRIBUTING.md :parser: myst_parser.sphinx_🧰 Tools
🪛 LanguageTool
[style] ~10-~10: Would you like to use the Oxford spelling “summarized”? The spelling ‘summarised’ is also correct.
Context: ...he steps for a new piece of work can be summarised as follows: 1. Push up or create [an i...(OXFORD_SPELLING_Z_NOT_S)
[style] ~61-~61: Consider removing “of” to be more concise
Context: ...are finished with the work, ensure that all of the unit tests pass on your own machine by ...(ALL_OF_THE)
[style] ~78-~78: Consider shortening or rephrasing this to strengthen your wording.
Context: ...uld lead to merge conflicts if you have made changes to the master branch. ## Pull requests F...(MAKE_CHANGES)
[uncategorized] ~82-~82: Possible missing comma found.
Context: ...eral overview of using pull requests on GitHub look [in the GitHub docs](https://help....(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CONTRIBUTING.md
(3 hunks)docs/CONTRIBUTING.md
(1 hunks)docs/index.rst
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/CONTRIBUTING.md
[style] ~10-~10: Would you like to use the Oxford spelling “summarized”? The spelling ‘summarised’ is also correct.
Context: ...he steps for a new piece of work can be summarised as follows: 1. Push up or create [an i...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~61-~61: Consider removing “of” to be more concise
Context: ...are finished with the work, ensure that all of the unit tests pass on your own machine by ...
(ALL_OF_THE)
[style] ~78-~78: Consider shortening or rephrasing this to strengthen your wording.
Context: ...uld lead to merge conflicts if you have made changes to the master branch. ## Pull requests F...
(MAKE_CHANGES)
[uncategorized] ~82-~82: Possible missing comma found.
Context: ...eral overview of using pull requests on GitHub look [in the GitHub docs](https://help....
(AI_HYDRA_LEO_MISSING_COMMA)
CONTRIBUTING.md
[style] ~61-~61: Consider removing “of” to be more concise
Context: ...are finished with the work, ensure that all of the unit tests pass on your own machine by ...
(ALL_OF_THE)
[style] ~78-~78: Consider shortening or rephrasing this to strengthen your wording.
Context: ...uld lead to merge conflicts if you have made changes to the master branch. ## Pull requests F...
(MAKE_CHANGES)
🔇 Additional comments (2)
docs/index.rst (1)
60-60
: LGTM! Good addition to the documentation structure.The CONTRIBUTING entry is properly added to the toctree, making the contribution guidelines easily accessible through the documentation.
CONTRIBUTING.md (1)
38-43
: Verify the optional dependencies.Let's verify that all optional dependencies are properly defined in setup.py.
✅ Verification successful
Optional dependencies are properly defined in pyproject.toml
The optional dependencies mentioned in CONTRIBUTING.md (
.[dev,docs,optional]
) are all correctly defined in the project configuration file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if optional dependencies are defined in setup.py rg -A 5 'optional.*=.*\[' setup.pyLength of output: 85
Script:
#!/bin/bash # Find package configuration files echo "=== Package configuration files ===" fd -e toml -e cfg -e py "^(pyproject|setup)" # If files are found, search for optional dependencies echo -e "\n=== Optional dependencies configuration ===" fd -e toml -e cfg -e py "^(pyproject|setup)" -x cat {}Length of output: 9391
Update continuous integration and contributing guide
Description
Type of change
How Has This Been Tested?
N/A
Test Configuration:
Reviewers
N/A
Checklist
Summary by CodeRabbit