- 
                Notifications
    
You must be signed in to change notification settings  - Fork 14
 
drop support for Python 2.x as it is very much EOL #22
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of more places needs to be updated as well:
Lines 124 to 126 in 1e6f644
| Starting with Kconfiglib version 12.2.0, all utilities are compatible with both | |
| Python 2 and Python 3. Previously, ``menuconfig.py`` only ran under Python 3 | |
| (i.e., it's now more backwards compatible than before). | 
Line 36 in 1e6f644
| implementation in Python 2/3. It started out as a helper library, but now has a | 
Lines 167 to 174 in 1e6f644
| Python version compatibility (2.7/3.2+) | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |
| Kconfiglib and all utilities run under both Python 2.7 and Python 3.2 and | |
| later. The code mostly uses basic Python features and has no third-party | |
| dependencies, so keeping it backwards-compatible is pretty low effort. | |
| The 3.2 requirement comes from ``argparse``. ``format()`` with unnumbered | 
Please check for more locations, those were just the ones I found on a quick search.
          
 ouch, yes, "a couple" more indeed :) will actually move to draft as I suspect it might take me a few attempts to get everything right  | 
    
A follow-up to b96a5ad where we stopped having CI run tests on Python 2.x. This actually drops the few remaining Python 2.x compatibility bits as Python 2.x has EOL'd a long time ago. Signed-off-by: Benjamin Cabé <[email protected]>
| 
           @tejlmand - it should be better now! It was definitely more than "a couple" :)  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Only some minor nits.
Have this PR been verified on Windows ?
| Kconfiglib and all utilities run under Python 3.9 and later. The code mostly | ||
| uses basic Python features and has no third-party dependencies. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we mention that it might work on older Python versions, but this is not tested / guaranteed to work ?
| # On Python 3 versions before 3.6, it's not possible to specify the | ||
| # encoding when passing universal_newlines=True to Popen() (the 'encoding' | ||
| # parameter was added in 3.6), so we do this manual version instead. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think keeping this comment make sense as it describes the reason for this construct.
Alternative, re-write the subprocess.run(...) to include universal_newlines=True and simplify this construct.
Side-note: seems the use of run() also means Python >=3.5 is now required 😉
| stderr = stderr.decode(kconf._encoding) | ||
| except UnicodeDecodeError as e: | ||
| _decoding_error(e, kconf.filename, kconf.linenr) | ||
| result = subprocess.run( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding universal_newlines=True ?
A follow-up to b96a5ad where we stopped having CI run tests on Python 2.x. This actually drops the few remaining Python 2.x compatibility bits as Python 2.x has EOL'd a long time ago.