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

python, linting: Align ruff with upstream #6066

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

austin3dickey
Copy link
Contributor

@austin3dickey austin3dickey commented Jan 21, 2025

Addresses #5667. This PR aligns the positron-python pyproject.toml with the upstream, which includes a lot of ruff linting.

Release Notes

New Features

  • N/A

Bug Fixes

  • N/A

QA Notes

This is a fairly big code change to the Python extension, but it's all linting, so I don't expect any behavior differences. I ran a full suite of tests. So I think we just want to be sure that Python comes up and we can use the kernel/console in a release build.

Copy link

github-actions bot commented Jan 21, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical

readme  valid tags

@austin3dickey
Copy link
Contributor Author

austin3dickey commented Jan 21, 2025

good start: Found 902 errors (540 fixed, 362 remaining).

@austin3dickey austin3dickey force-pushed the aus/ruff branch 3 times, most recently from 618b518 to 4790fdd Compare January 23, 2025 19:48
@austin3dickey austin3dickey marked this pull request as ready for review January 23, 2025 20:27
@austin3dickey austin3dickey changed the title [WIP] python, linting: Align ruff with upstream python, linting: Align ruff with upstream Jan 23, 2025
@austin3dickey
Copy link
Contributor Author

This should be good to go; CI is green (though to be fair I'm still combing through the diff to make sure there's nothing unexpected 😄)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now this looks like the upstream plus a couple fences.

seeM
seeM previously approved these changes Jan 24, 2025
Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for taking this one for the team 😄 I suggest manually running the E2E testing GH action against this branch before merging to main as one added precaution given the size of this PR.

isabelizimm
isabelizimm previously approved these changes Jan 24, 2025
Copy link
Contributor

@isabelizimm isabelizimm left a comment

Choose a reason for hiding this comment

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

everything looks great! this work is so appreciated, thank you for grabbing this issue!

post #6085 merge it does look like there are a bunch of conflicts 😬 lmk if you have any questions, it should just be renaming files 🤞

@austin3dickey
Copy link
Contributor Author

I suggest manually running the E2E testing GH action

Good idea; kicked off a full suite here

it does look like there are a bunch of conflicts

Luckily it wasn't bad at all! I don't know why the GitHub UI showed so many. Locally I only had to fix a couple imports in like 6 files. I didn't have to rename anything 🙂

Copy link
Contributor

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

Full test suite is green! ✅

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.

4 participants