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

Deprecate sage.misc.misc.sage_makedirs #32987

Closed
mkoeppe opened this issue Dec 6, 2021 · 17 comments
Closed

Deprecate sage.misc.misc.sage_makedirs #32987

mkoeppe opened this issue Dec 6, 2021 · 17 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 6, 2021

As noted in #29093, in py3, os.makedirs learned an exist_ok option that makes sage_makedirs entirely redundant.

https://docs.python.org/3/library/os.html#os.makedirs

Since then we have dropped py2 support, so we can deprecate sage_makedirs and replace all uses in the library.

CC: @mezzarobba @orlitzky

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: d3b3252

Reviewer: Kwankyu Lee, Michael Orlitzky

Issue created by migration from https://trac.sagemath.org/ticket/32987

@mkoeppe mkoeppe added this to the sage-9.5 milestone Dec 6, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2021

Dependencies: #32986

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 9, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 9, 2021

Commit: 37218da

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 9, 2021

New commits:

81d0fffsage.misc.temporary_file: Move SAGE_TMP implementation here
7e78f59src/sage/misc/temporary_file.py: Remove use of functools.cache
a9de636Merge #32986
52b810fgit grep -l sage_makedirs | xargs sed -E -i.bak 's/from sage.misc.misc import sage_makedirs/import os/;s/sage_makedirs[(](.*)[)]$/os.makedirs(\1, exist_ok=True)/'
5b94d90Remove duplicate imports
976ff6fRemove remaining import of sage_makedirs
37218dasage.misc.misc.sage_makedirs: Deprecate

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 9, 2021

Author: Matthias Koeppe

@kwankyu
Copy link
Collaborator

kwankyu commented Dec 10, 2021

comment:5

It works well.

I see two doctest failures with src/sage/tests/cmdline.py, but these seem related with pytest issues, not with the current branch.

@kwankyu
Copy link
Collaborator

kwankyu commented Dec 10, 2021

Reviewer: Kwankyu Lee

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 10, 2021

comment:6

Thank you!

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Jan 12, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 18, 2022

Changed commit from 37218da to d3b3252

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 18, 2022

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

766d835git grep -l sage_makedirs | xargs sed -E -i.bak 's/from sage.misc.misc import sage_makedirs/import os/;s/sage_makedirs[(](.*)[)]$/os.makedirs(\1, exist_ok=True)/'
4494364Remove duplicate imports
41b015cRemove remaining import of sage_makedirs
e891787sage.misc.misc.sage_makedirs: Deprecate
d3b3252sage.misc.misc (SAGE_TMP, ECL_TMP, SAGE_TMP_INTERFACE): Also replace use of sage_makedirs here

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 18, 2022

Changed dependencies from #32986 to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 18, 2022

comment:9

Rebased away from #32986 (which is stuck)

@orlitzky
Copy link
Contributor

Changed reviewer from Kwankyu Lee to Kwankyu Lee, Michael Orlitzky

@orlitzky
Copy link
Contributor

comment:10

This was already positively reviewed, but LGTM too after rebasing.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 19, 2022

comment:11

Thanks!

@vbraun
Copy link
Member

vbraun commented Feb 21, 2022

Changed branch from u/mkoeppe/deprecate_sage_misc_misc_sage_makedirs to d3b3252

@vbraun vbraun closed this as completed in 347ee93 Feb 21, 2022
mkoeppe added a commit to mkoeppe/sage that referenced this issue Apr 25, 2024
mkoeppe added a commit to mkoeppe/sage that referenced this issue Apr 27, 2024
vbraun pushed a commit to vbraun/sage that referenced this issue May 2, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Remove code deprecated in
- sagemath#32987 (2022)
- sagemath#33213 (2022)
- sagemath#29869 (2020)
- sagemath#17815 (2020)
- sagemath#29892 (2020)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37868
Reported by: Matthias Köppe
Reviewer(s): Michael Orlitzky
@mantepse mantepse mentioned this issue May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants