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

Move more packages to the new primer #6753

Merged
merged 1 commit into from
May 30, 2022

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

I'll remove the empty file when we can remove the old primer as removing now requires additional refactors.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2410153713

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.516%

Totals Coverage Status
Change from base Build 2409548505: 0.0%
Covered Lines: 16230
Relevant Lines: 16992

πŸ’› - Coveralls

@jacobtylerwalls jacobtylerwalls merged commit 896c8ed into pylint-dev:main May 30, 2022
@DanielNoord DanielNoord deleted the primer-packages branch May 30, 2022 20:24
@DanielNoord
Copy link
Collaborator Author

@jacobtylerwalls This is running into CI time-outs. See main.

I don't really understand as this batch of packages passed on my local branch in CI and locally. Does this pass in reasonable time for you locally?

@jacobtylerwalls
Copy link
Member

Make sure to try with bleeding-edge astroid, it has good nutrients :D

@DanielNoord
Copy link
Collaborator Author

Make sure to try with bleeding-edge astroid, it has good nutrients :D

Does bleeding-edge fail with these packages? 😒

@jacobtylerwalls
Copy link
Member

Well, I'm waiting to find out. It takes a long time to lint home-assistant! How long does it take on your system?

@DanielNoord
Copy link
Collaborator Author

DanielNoord commented May 30, 2022

I got 21m in CI here:
https://github.com/DanielNoord/pylint/runs/6657709454?check_suite_focus=true
Note that that is with:
DanielNoord@e739427

I removed keras, Django and music21 for this PR as they all crashed due to the importlib issue.

@jacobtylerwalls
Copy link
Member

@DanielNoord the other thing I can think of is the message dict might be huge. Could we write the message dict and flush it as each package is linted, rather than writing it out after all packages? This will also help with debugging.

@DanielNoord
Copy link
Collaborator Author

Now that I look at that workflow again I see that we never actually go beyond keras. So it was probably broken as well, but just failed prematurely. I didn't recognise that correctly... Sorry!

@DanielNoord the other thing I can think of is the message dict might be huge. Could we write the message dict and flush it as each package is linted, rather than writing it out after all packages? This will also help with debugging.

Yeah that should be do-able.
The only thing we need to take care of is writing { before the first package , after each package and } after the last package to make the final output a valid JSON dictionary.

I was actually planning on shutting down for the night. If you want feel free to take a stab at this or just revert this PR so main gets unblocked. I think I won't get to this before tomorrow evening (+24h from now).

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented May 31, 2022

Woo hoo! The issue with pandas is fixed at pylint-dev/astroid#1579.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants