Skip to content

trying to avoid env installation for ruff#2

Merged
SmartMonkey-git merged 13 commits intomainfrom
lc/fixruffdeps
Aug 1, 2025
Merged

trying to avoid env installation for ruff#2
SmartMonkey-git merged 13 commits intomainfrom
lc/fixruffdeps

Conversation

@leokim-l
Copy link
Collaborator

As we were discussing elsewhere:

"The idea of skipping to set up the whole environment came from here, all the way at the bottom (they do it with tox.ini but that does not matter):
https://pythonspeed.com/articles/pylint-flake8-ruff/

But we are already caching the environment so I don't expect much difference in practice."

@leokim-l leokim-l requested a review from SmartMonkey-git July 23, 2025 17:31
@leokim-l
Copy link
Collaborator Author

As mentioned, with a cached environment this is probably not super important, but I'd still avoid loading/risking installing a whole environment if it's not needed. Thanks again Rouven for all of this, so cool! :)

Copy link
Owner

@SmartMonkey-git SmartMonkey-git left a comment

Choose a reason for hiding this comment

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

Looks good. Only two wishes ✨

Comment on lines +11 to +20
- name: Get project version with yq
id: get_python_version
uses: mikefarah/yq@v4.46.1
with:
cmd: yq '.project.requires-python' pyproject.toml

- name: Set up Python
uses: actions/setup-python@v5.6.0
with:
python-version: ${{ steps.get_python_version.outputs.result }}
Copy link
Owner

Choose a reason for hiding this comment

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

Would not add them in here. Either another action or put them directly into the jobs. Since, when I run this action, I would not expect it to setup python for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure I understand, we need python and pip to install and run ruff... I think I am misunderstanding your comment

Copy link
Owner

@SmartMonkey-git SmartMonkey-git Jul 27, 2025

Choose a reason for hiding this comment

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

Yes, you need python, but it would also be unexpected if it installed the version found in my pyproject toml, because it promises just install a package.

@SmartMonkey-git
Copy link
Owner

Actually, I found that there is a ruff action, that does already, what you implemented. :/

https://github.com/marketplace/actions/ruff-action

Should we keep what you have?

@SmartMonkey-git SmartMonkey-git changed the base branch from rr/setup to main July 27, 2025 09:35
@@ -41,7 +41,10 @@ jobs:
fetch-depth: 0

- name: Setup Env
Copy link
Owner

Choose a reason for hiding this comment

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

This and the linting action is not named correctly. :)

with:
cmd: yq '.project.requires-python' pyproject.toml

- name: Set up Python
Copy link
Owner

Choose a reason for hiding this comment

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

Can you do a check here if python is already installed?

Copy link
Owner

Choose a reason for hiding this comment

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

@SmartMonkey-git SmartMonkey-git merged commit 200b272 into main Aug 1, 2025
3 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.

2 participants