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

CLEANUP: cleanup tox.ini, and ignore ./env #1200

Merged
merged 7 commits into from
Jun 2, 2023
Merged

CLEANUP: cleanup tox.ini, and ignore ./env #1200

merged 7 commits into from
Jun 2, 2023

Conversation

alifeee
Copy link
Collaborator

@alifeee alifeee commented May 31, 2023

tox was taking a long time to run. This was because it was checking all the files in my python virtual environment, in ./env.

The .gitignore suggests that a python virtual environment should be called env, which is what mine is.

I have added a skip/ignore to the lint/format commands, so they no longer look in ./env.

This also has the benefit that they are much faster to run (around 8 secs to lint, 5 secs to format).

@lavigne958
Copy link
Collaborator

Thanks that's great ! 😃

I noticed it too.

I thought about the following changes: using the command git ls-files *.py in order to only format/lint tracked files.

I mostly faced issues when it tries to lint my dev files that are not formatted at all.

We can mix your proposal with mine in this PR.

@alifeee
Copy link
Collaborator Author

alifeee commented May 31, 2023

I thought about the following changes: using the command git ls-files *.py in order to only format/lint tracked files.

And changing file exclusions to file inclusions? That sounds smart.

@lavigne958
Copy link
Collaborator

I let you do the changes in your PR?

Remove the exclude list in lint/format. Then instead of . for the folder to scan, replace it with $(git ls-files *.py)

Hope this works but I'm not sure if tow runs the commands in a shell or not.

@lavigne958
Copy link
Collaborator

In fact I was wrong we don't have access to a shell we should keep it like this for now (in terms of exclude/include).

We may introduce exclude list to black, like the others: .tox, env, docs, ...

@alifeee
Copy link
Collaborator Author

alifeee commented Jun 1, 2023

In fact I was wrong we don't have access to a shell we should keep it like this for now (in terms of exclude/include).

Ah, ok. Sounds good.

I let you do the changes in your PR?

In theory, you should be able to make changes in my repository, since you are a maintainer.

image

However, I will make this small change.

I think that black is smart anyway, and does not re-compute files it has already done once.
Although perhaps that would be an issue in a cache-less CI job
commands already include a nice set of defaults. I'd rather not override them.
--extend-include allows you to add more to the list
see "flake8 --help", "black --help", or "isort --help" for more
@alifeee
Copy link
Collaborator Author

alifeee commented Jun 1, 2023

I believe this can be merged 👍

@lavigne958 lavigne958 merged commit 5a93446 into burnash:master Jun 2, 2023
6 checks passed
@alifeee alifeee deleted the tox-ignore-more branch June 2, 2023 13:31
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.

None yet

2 participants