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 3 cleanups #824

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

eli-schwartz
Copy link
Contributor

Roughly divided into two changes:

  • remove the use of the future PyPI package, which is broken on python 3.12 and unneeded in python 3 entirely.
  • perform various code cleanups and drop python2-specific code

The first part is critical to allow running on modern versions of python. The second part I did just because I can, so why not.

Ironically, for a package that was intended to provide portability
between python2 and python3, it is broken with python 3.12. A better
library to use in all cases is "six".

However, mythtv requires python 3.8 for a while now. Using
"future.standard_library" is a no-op other than costing a pointless
import and being troublesome to actually install.

The hacky copy of six.with_metaclass included in "future" is rewritten
to use the pure python3 form of a metaclass.
Drop ancient and outdated backwards compatibility code designed to
support python2, which is no longer supported anymore.

Performed using the command:
```
find . -name '*.py' -exec pyupgrade --py3-only --keep-percent-format {} +
```

and committing the results.

Mostly, this changes:
- remove coding cookies
- drop no-op imports of `__future__` or `builtins`
- classes don't need to inherit object anymore
- simpler `super()`
- drop the no-op string prefix u''
- drop some code under `sys.version_info == 2`. This never worked
  because it didn't compare to .major, but if it did work it would not
  be needed anymore on python 3.
The bulk of this is to just manually unroll custom is_py3 checks and
switch to the canonical python3 naming. If this had just used six, then
this would be trivially automated...
@rcrdnalor
Copy link
Contributor

rcrdnalor commented Dec 28, 2023

Yes, python 3.12 removes the imp module but python-future v0.18.3 uses it, see
PythonCharmers/python-future#625
This statement is currently broken:
from future import standard_library
Since the hardware profiler only uses above import and I have no clue about this module
@billmeek , could you please have a look for 0efec84 ?
Note: The __future__ module is a different module than the future module, thus only the first commit of this PR is of importance.

@a-detiste
Copy link

'from future import standard_library' enabled to run Py3 code under Py2. Totally unnecessary now.

@rcrdnalor
Copy link
Contributor

rcrdnalor commented Jan 4, 2024

@eli-schwartz Thank you for providing these patches!
Unfortunately, this was the easy part, because the
python modules of mythtv are maintained by different
persons (for good reasons).

My suggestions for separate pull requests:

  • Strip off the first commit (hardwareprofiler)
  • Consider a second PR that removes python-simplejson,
    because it is used only once in
    scripts/hardwareprofile/smolt.py as
    from simplejson import errors as sje

I would consider above PRs as necessary for the upcoming
release of mythtv v.34

  • Add separate PRs for
    • programs/scripts/hardwareprofile
    • programs/scripts/internetcontent
    • contrib/imports/mirobridge
    • mythgame/scripts
    • mytharchive/scripts
    • metadata/Music
    • metadata/Televison or Movie
    • build scripts

Rationale: Some of above modules are not functional

  • Omit stuff in external like ffmpeg or msvc
    because they are only pulled in.

  • Merge the musicbrainz python modules with
    the latest provided by musicbrainz and
    open a PR on their side

My rationale for this separation is to
'logically organize commits' as proposed by MythTVs policies.

@eli-schwartz
Copy link
Contributor Author

My original interest in this is as a Linux distro packager, not a mythtv user -- this is a QA blocker for marking all packages as 3.12 compatible and eventually phasing out 3.10 and 3.11 support -- so I lack the familiarity to be aware in advance of that module maintainer split.

Your explanation does make a ton of sense, and I'd be more than delighted to accommodate it. It makes a great change from having my careful patches squash-merged. :P

Question: for the automatic tool-powered changes, do you want me to apply one autofix type per commit? It's somewhat easier to review that way but either approach isn't hard, so I'm not sure which you prefer.

@rcrdnalor
Copy link
Contributor

rcrdnalor commented Jan 4, 2024

I really appreciate Your help in this topic!
First, I want to solve the problem with pythons 3.12 future import, which is a major issue for the upcoming release.
Second, I prefer the modules which are known as functional.
Third, i do not prefer to pimp-up modules known as not functional. This makes them more visible for needed maintenance.

@eli-schwartz
Copy link
Contributor Author

I submitted 2 PRs containing the broken-out commits for simplejson and future.

@rcrdnalor
Copy link
Contributor

I submitted 2 PRs containing the broken-out commits for simplejson and future.

First parts committed, thank you for help!
Let's see what the Ubuntu ppa for v34 tells us tomorrow ( it is in the pythons future :-) ).

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.

3 participants