-
Notifications
You must be signed in to change notification settings - Fork 133
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
examples: added pre-commit check and format all notebooks #1220
Conversation
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.
❌ Changes requested. Reviewed everything up to c707be3 in 1 minute and 6 seconds
More details
- Looked at
235
lines of code in2
files - Skipped
69
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. examples/validate_examples.py:108
- Draft comment:
Consider allowing the dependencies to be specified as a parameter to make this function more flexible. - Reason this comment was not posted:
Confidence changes required:50%
The functioninsert_setup_cell
hardcodes the dependencies to be installed. It would be more flexible to allow specifying dependencies as a parameter.
2. examples/validate_examples.py:131
- Draft comment:
Consider reusing_create_github_badge
and_create_colab_badge
to avoid code duplication. - Reason this comment was not posted:
Confidence changes required:50%
The functionadd_badges_to_title
duplicates the logic for creating badges. It would be better to reuse the existing functions_create_github_badge
and_create_colab_badge
.
Workflow ID: wflow_OWcqVJs2Cjpf0tXB
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
return colab_badge | ||
|
||
|
||
def validate_notebook(notebook_path: pathlib.Path) -> int: |
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 validate_notebook
function is performing multiple checks. Consider refactoring it into smaller functions, each handling a specific check, to adhere to the Single Responsibility Principle.
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.
👍 Looks good to me! Incremental review on a6f651e in 30 seconds
More details
- Looked at
32
lines of code in1
files - Skipped
1
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. examples/validate_examples.py:128
- Draft comment:
Ensure that badges are not duplicated if they already exist in the markdown cell. Consider checking for existing badges before adding new ones. - Reason this comment was not posted:
Comment did not seem useful.
2. examples/validate_examples.py:133
- Draft comment:
The logic for creating badges is repeated in multiple places. Consider refactoring the badge creation logic into a separate function to adhere to the DRY principle. This applies to lines 18-20 and 24-27 as well. - Reason this comment was not posted:
Comment was on unchanged code.
3. examples/validate_examples.py:128
- Draft comment:
Theadd_badges_to_title
function is handling multiple responsibilities. Consider refactoring it to separate the reading, updating, and writing of the notebook into distinct functions to adhere to the Single Responsibility Principle. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_qioxCvTJddbfHIET
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Follows #1212.
This adds a script
examples/validate_examples.py
to check that all.ipynb
files underexamples/
follow the rules:%pip install
to install dependencies from the notebook / Colab environment.## ignore_ci
, checks are skippedEXCLUDED_EXAMPLES
, it is ignored (equivalent to## ignore_ci
)Notes
%pip
, which is the current venv and!pip
uses the base environment.%pip
has limitations for environment managers that don't includepip
in their venv (e.g.,uv
,pdm
). In that case, the user is responsible for adding dependencies from the terminal.