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

Updates after dropping Python 3.6 and 3.7 #1372

Merged
merged 3 commits into from
Jul 23, 2023
Merged

Conversation

hugovk
Copy link
Contributor

@hugovk hugovk commented Jul 22, 2023

Fix some things left over from the drop of EOL Python 3.6 and 3.7:

# TODO: Py3.6 compatibility; os.fsdecode not required in Py3.7

Replace os.fsdecode() with str(), because they can be pathlib Paths.

Also need things like archive.__str__.return_value = "/path/to/download.zip" for the MagicMock objects.


    # Consider to remove if block when we drop py3.7 support, only keep statements from else.
    # MagicMock below py3.8 doesn't have __fspath__ attribute.

The if/else blocks (they used to look like 05c50b9) have already been cleaned up (presumably by pyupgrade), so remove these old comments.

PR Checklist:

  • All new features have been tested
  • [n/a] All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The cleanups of Py3.6/3.7 detritus are definitely required and appreciated; however, the usage of os.fsdecode is deliberate. This is based on advice from Brett Cannon, (provided in many places; this is one example), my understanding is that os.fsdecode() is preferred for rendering Pathlib objects, in particular because of None handling.

If you've got a different understanding, let us know what I've missed (or misunderstood).

@hugovk
Copy link
Contributor Author

hugovk commented Jul 23, 2023

Ah right, I was mainly going on the comments, so will re-instate the os.fsdecode calls.

And keep the removal of the # TODO: Py3.6 compatibility; os.fsdecode not required in Py3.7 comments?

@freakboy3742
Copy link
Member

And keep the removal of the # TODO: Py3.6 compatibility; os.fsdecode not required in Py3.7 comments?

Those comments can all be burned :-) You guessed correctly that they were all left in because pyuprade removed an if/else branch, but didn't pick up the comment explaining the branch.

@hugovk
Copy link
Contributor Author

hugovk commented Jul 23, 2023

Pushed! Let me know if you'd prefer a fixup/rebase.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Awesome - thanks for the 🧹 !

@freakboy3742 freakboy3742 merged commit 080d6cb into beeware:main Jul 23, 2023
16 of 19 checks passed
@hugovk hugovk deleted the rm-eol branch July 23, 2023 10:15
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.

2 participants