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

Enable pylint for the python tests #11600

Closed
wants to merge 1 commit into from

Conversation

gigiblender
Copy link
Contributor

This PR enables pylint for the tests and addresses #11414.

There is a large number of tests that have formatting errors. See the file attached.

out.txt

@Mousius
Copy link
Member

Mousius commented Jun 7, 2022

@gigiblender is there anyway to make it error for new tests?

@gigiblender
Copy link
Contributor Author

@Mousius I don't see a clean way to do this.

We could have a temporary pylintrc file, used only for the tests, that passes the list in #11414 as the ignore parameter.. However, new tests in existing files would also be ignored in this case.

@Mousius
Copy link
Member

Mousius commented Jun 9, 2022

@Mousius I don't see a clean way to do this.

We could have a temporary pylintrc file, used only for the tests, that passes the list in #11414 as the ignore parameter.. However, new tests in existing files would also be ignored in this case.

Hmmm, I'm unsure whether we want to fill the logs with pylint without actually halting the build? Methinks it's worth doing as you suggested and filtering out the files we know will fail - we then have to lint them ourselves as quickly as we can 😿

@gigiblender gigiblender force-pushed the pylint-tests branch 3 times, most recently from bf92d5e to 72fe47a Compare June 10, 2022 14:40
@gigiblender
Copy link
Contributor Author

@Mousius I don't see a clean way to do this.
We could have a temporary pylintrc file, used only for the tests, that passes the list in #11414 as the ignore parameter.. However, new tests in existing files would also be ignored in this case.

Hmmm, I'm unsure whether we want to fill the logs with pylint without actually halting the build? Methinks it's worth doing as you suggested and filtering out the files we know will fail - we then have to lint them ourselves as quickly as we can crying_cat_face

@Mousius, I applied your suggestion. Please have a look when you get the time.

@Mousius
Copy link
Member

Mousius commented Jun 15, 2022

Argh @gigiblender, I didn't realise we would only be able to specify test_pooling.py, that means test_cmsisnn is ignored by mistake in the last test block - ideally we'd disable all of them and then enable them as we merge PRs to fix them, unsure if we can do that or if we just have to keep adding directories to the list and clean up later 🤔 any ideas @gigiblender ?

@gigiblender
Copy link
Contributor Author

@Mousius, I think the second option sounds better. We can add subdirectories to the list as we fix them and collapse the list in a single entry once all the tests pass the linter. I think I can close this PR.

@Mousius
Copy link
Member

Mousius commented Jun 15, 2022

Sounds good to me @gigiblender, thanks for raising this and kickstarting the great linting of 2022!

@gigiblender gigiblender deleted the pylint-tests branch June 22, 2022 19:23
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