Skip to content
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

ruff instead of black / isort #541

Merged
merged 2 commits into from
Mar 19, 2024
Merged

ruff instead of black / isort #541

merged 2 commits into from
Mar 19, 2024

Conversation

biobootloader
Copy link
Member

Pull Request Checklist

  • Documentation has been updated, or this change doesn't require that

run: black --check --preview .
- name: isort check
run: isort --profile black --check .
curl -LsSf https://astral.sh/uv/install.sh | sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good practice to verify the integrity of the script before executing it. Consider adding a checksum verification step for the install.sh script to ensure its authenticity and integrity.

run: isort --profile black --check .
curl -LsSf https://astral.sh/uv/install.sh | sh
uv venv
uv pip install -r dev-requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using uv pip install directly might not be clear to everyone. Consider adding a comment explaining that uv is a tool for managing Python environments and that this command installs dependencies in a virtual environment.

- name: activate venv
run: |
. .venv/bin/activate
echo PATH=$PATH >> $GITHUB_ENV
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appending to the PATH environment variable directly in the script can lead to unexpected behavior in complex workflows. If possible, consider using environment files or other GitHub Actions features for setting environment variables.

- name: ruff (linter)
run: ruff check .
- name: ruff (formatter)
run: ruff format --check .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ruff format --check . command is used to check formatting without making changes. If the intent is to also format the code, consider running ruff format without the --check flag in a separate step that only runs on a specific branch or conditionally, to avoid unintended code changes in pull requests.

Copy link
Contributor

mentatbot bot commented Mar 19, 2024

MENTAT CODE REVIEW IN ACTIVE DEVELOPMENT. Only in use on mentat and internal repos.
Please Reply with feedback.

The switch to using ruff for linting and formatting is a significant change. Overall, the implementation looks solid, but there are a few areas where clarity and security could be improved. It's also important to ensure that contributors are aware of this change and understand how to run these checks locally to reduce CI failures.

@biobootloader biobootloader marked this pull request as ready for review March 19, 2024 04:08
@biobootloader biobootloader force-pushed the ruff-only branch 2 times, most recently from c7a15fb to 04c8f20 Compare March 19, 2024 21:35
@biobootloader biobootloader merged commit f0edb8d into main Mar 19, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant