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

PR: Avoid future deprecations and decrease general technical debt #273

Merged

Conversation

CAM-Gerlach
Copy link
Member

Fixes a number of minor problems in the codebase, including:

  • Replace the deprecated tmpdir pytest fixture which uses legacy py.path objects with tmp_path, which uses standard pathlib.Paths (and rename helper function arg to avoid confusion)
  • Use explicit encoding on open(), to avoid platform/environment-dependent bugs, deprecation warning on Py3.10 and eventual removal
  • Fix other correctness issues, including:
    • Bad indent levels in some lines/blocks
    • Missing raise from on re-raised exceptions to re-raise correctly
    • Bare except that will trap KeyboardInterrupt, termination and other unwanted errors
    • Misplaced strings intended as docstrings but not the first line in their module/callable

@dalthviz
Copy link
Member

dalthviz commented Nov 3, 2021

Hi @CAM-Gerlach thanks for checking into this! However, just to understand better some of the changes here, the deprecations (pytest fixture, open with enconding kwarg) are related with Python 3.10?

@CAM-Gerlach
Copy link
Member Author

@dalthviz Sure:

  • The Pytest tmpdir fixture has been targeted for removal in Pytest for some time as it relies on the long-deprecated py package and legacy py.path objects; there's no connection to Python version. See Please remove the dependency on 'py' pytest-dev/pytest#7259
  • The lack of an explicit encoding kwarg on open will become an optional warning in Python 3.10, with the plan to be a DeprecationWarning in future Python versions, and possibly eventually an error. However, the problem it fixes is present in all past and present Python versions, namely that the current default encoding is platform, environment and user-dependent (e.g. on Windows, some Linux and other platforms it is not UTF-8, as many developers assume, and can be anything, potentially not even ASCII-compatible) and is unsafe to rely upon, as attempting to open files in an arbitrary encoding is liable to cause an error or undesired behavior. See PEP 597 for more details (N.B., I helped some with the PEP and docs, and a bit with the implementation).
  • The other changes are all relatively smaller but non-trivial correctness issues that have actual runtime impact and don't directly involve a deprecation, they've been problems ab initio

@dalthviz dalthviz changed the title PR: Upgrade/fix various deprecated, unsafe and/or bad practice constructs PR: Fix future deprecations and decrease general technical debt and code style issues Nov 4, 2021
@dalthviz dalthviz changed the title PR: Fix future deprecations and decrease general technical debt and code style issues PR: Prevent future deprecations messages and decrease general technical debt + code style issues Nov 4, 2021
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @CAM-Gerlach ! LGTM 👍

@dalthviz dalthviz merged commit f26d9e9 into spyder-ide:master Nov 4, 2021
@ccordoba12 ccordoba12 added this to the v2.0.0 milestone Nov 4, 2021
@CAM-Gerlach CAM-Gerlach changed the title PR: Prevent future deprecations messages and decrease general technical debt + code style issues PR: Avoid future deprecations and decrease general technical debt Nov 4, 2021
@CAM-Gerlach
Copy link
Member Author

Thanks @dalthviz ; revised the PR title again to fix some introduced grammar and clarity issues

@CAM-Gerlach CAM-Gerlach deleted the upgrade-deprecated-tmpdir-open branch December 1, 2021 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants