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

GitHub Actions & Dev Tools Cleanup + Python 3.11 Support #1009

Closed
wants to merge 16 commits into from

Conversation

bmos
Copy link
Contributor

@bmos bmos commented Mar 12, 2024

I'm not really sure how these changes could be merged nicely with other open PRs.
I'm happy to restructure it however is desired, so perhaps treat this as a sort of menu.

What's Included

  • Simplified the linting workflow to use a single file for both mac and linux testing.
  • Use the rust-based package installer uv to significantly reduce the GitHub actions runtime to allow a second python version in macOS testing.
  • Functionalized __init__.py (which is kind of pointless but I did it while debugging an error I was getting) and fixed a typo in it (one of the tuples was a list instead).
  • Fix import of HttpError in gmail.py (which for some reason your pytest workflow was not catching).
  • Updated packages that were needed to get pytest to pass or were missing patch-level updates.
  • Restructured limited dependencies mode to use the same dependency versions as the usual install.
  • Resolve the TypeError raised when running pytest under Python 3.11. Required upstream fix.
  • Flake8 was set to maintain a 100 character max line length, but Black didn't have a line length option configured which meant it was defaulting to a much lower 88. These are now both 100.
  • Add flake8, pytest, and black configurations to pyproject.toml rather than having a mix of separate config files and command-line args -- also standardized the commands used in all places.
  • Updated Black and Flake8 to latest versions and applied their updated linting suggestions.
  • Pytest now finishes with 3 warnings instead of 13.

Notes / Considerations

@bmos bmos force-pushed the patch-1 branch 4 times, most recently from fc61742 to 9091d96 Compare March 12, 2024 22:00
@bmos bmos force-pushed the patch-1 branch 3 times, most recently from 3557539 to d3ac21e Compare March 12, 2024 22:53
@bmos bmos marked this pull request as ready for review March 12, 2024 22:58
@bmos bmos changed the title GitHub Action + Dev Tools Cleanup GitHub Action + Dev Tools Cleanup + Python 3.11 Support Mar 12, 2024
@bmos bmos force-pushed the patch-1 branch 2 times, most recently from 7d69825 to 86d9fb4 Compare March 13, 2024 02:21
@bmos bmos marked this pull request as draft March 13, 2024 02:22
@bmos bmos changed the title GitHub Action + Dev Tools Cleanup + Python 3.11 Support GitHub Actions & Dev Tools Cleanup + Python 3.11 Support Mar 13, 2024
@bmos bmos marked this pull request as ready for review March 13, 2024 02:30
(cherry picked from commit a05473f)
@austinweisgrau
Copy link
Collaborator

austinweisgrau commented Mar 13, 2024

I think this should be split into several different PRs for the different components here. Each PR should explain the motivation for the relevant changes and what is affected.

I see these as separate PR scopes:

  • linting changes
  • CICD changes
  • package management changes
  • 3.11 compatibility
  • bug fixes

@austinweisgrau
Copy link
Collaborator

austinweisgrau commented Mar 13, 2024

Good work though, these look mostly like nice updates. I think we'd want to discuss pros and cons of switching to uv, and I think it would be preferable to stick to the black line length default of 88 rather than switching it to the flake8 default of 100, but we should have those discussions in smaller scoped PRs.

Another note is to make sure that any PRs with breaking changes are set up to merge into the major-release branch rather than main. Mostly looking at the dropping support for 3.7.

@bmos
Copy link
Contributor Author

bmos commented Mar 13, 2024

Thanks for those notes, I will restructure this into the suggested categories.

I think it would be preferable to stick to the black line length default of 88 rather than switching it to the flake8 default of 100

100 is not the default for flake8, that would be 79.
I went with 100 because the flake8 config file in your repo says 100 and the CONTRIBUTING.md file also includes it in the flake8 command. If 88 is the desired value I can change those values of 100 to 88 -- standardization was my goal, not a longer line length.

@austinweisgrau
Copy link
Collaborator

Thanks for those notes, I will restructure this into the suggested categories.

I think it would be preferable to stick to the black line length default of 88 rather than switching it to the flake8 default of 100

100 is not the default for flake8, that would be 79. I went with 100 because the flake8 config file in your repo says 100 and the CONTRIBUTING.md file also includes it in the flake8 command. If 88 is the desired value I can change those values of 100 to 88 -- standardization was my goal, not a longer line length.

Oh word gotcha that makes sense! Yeah I think maintaining whatever the documented value is makes the most sense so that sounds right to me.

@bmos bmos closed this Mar 14, 2024
@bmos bmos mentioned this pull request Mar 14, 2024
@bmos bmos deleted the patch-1 branch March 14, 2024 02:24
@bmos
Copy link
Contributor Author

bmos commented Mar 14, 2024

Does the major release branch need to be brought up to main?

@austinweisgrau
Copy link
Collaborator

austinweisgrau commented Mar 14, 2024 via email

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.

[Bug] Parsons 3.1.0 targets yanked 4.0.0 version of braintree with critical bugs
2 participants