Skip to content

Add home-assistant to primer and remove old external primer #8612

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

Merged
merged 5 commits into from
Apr 29, 2023

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

Restore home-assistant to the new primer, and finally remove the old (external) primer. The old (check for fatal only) primer still runs on the stdlib.

We were waiting for the 3.7 primer job to take less time before doing this, but we fixed that in #8608, and we're dropping 3.7 soon.

The commit string was too large for the cache action (512 chars ceiling), so I shortened the SHAs it uses.

Removed keras, as it wasn't linting successfully in this context, unfortunately.

Successful run on my fork

Closes #5359

@jacobtylerwalls jacobtylerwalls added primer Skip news πŸ”‡ This change does not require a changelog entry labels Apr 24, 2023
@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Merging #8612 (a0db1b9) into main (a83137d) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8612   +/-   ##
=======================================
  Coverage   95.91%   95.91%           
=======================================
  Files         174      174           
  Lines       18416    18416           
=======================================
  Hits        17664    17664           
  Misses        752      752           
Impacted Files Coverage Ξ”
pylint/testutils/_primer/primer_prepare_command.py 23.33% <ΓΈ> (ΓΈ)

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Isn't keras failing in the "new" primer a valid reason to have it in the primer ? (If we only add projects that work from the get go we're selecting projects that are somewhat more compatible with pylint than the general projects pool)

@jacobtylerwalls
Copy link
Member Author

Isn't keras failing in the "new" primer a valid reason to have it in the primer ? (If we only add projects that work from the get go we're selecting projects that are somewhat more compatible with pylint than the general projects pool)

Keras works locally when fewer projects are involved. We're blowing out available memory on Github Actions, I assume. We probably have too many projects in the primer anyway. If we ever parallelized the primer job, we could consider adding it back.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

πŸ‘Œ

@Pierre-Sassoulas
Copy link
Member

I was going to squash but it occurred to me that keeping the 4 commit separated in the history could be preferable, but there's a conflict so it's impossible to rebase. I'll let you choose what to do :)

@jacobtylerwalls
Copy link
Member Author

We can squash it; we don't need a super-fine resolution on a git blame for this testing tool...

@jacobtylerwalls jacobtylerwalls merged commit 87a4d67 into pylint-dev:main Apr 29, 2023
@jacobtylerwalls jacobtylerwalls deleted the remove-old-primer branch April 29, 2023 18:12
@DanielNoord
Copy link
Collaborator

I think this regressed? See #8638, primer can't run anymore.

@jacobtylerwalls
Copy link
Member Author

Taking a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
primer Skip news πŸ”‡ This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better primer tests (faster, less duplication, more diverse)
3 participants