Make overwrite=True work with Path arguments - #17552

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

I have investigated a bit more the issue on the test (see this comment) and decided to remove the check on the type of output.
I'm still puzzled about how to write an effective test because the issue also depends on the path of the output itself not only on the type of the variable.
I don't particularly like the idea of checking against multiple types... this is more a mypy job ...
Is that solution acceptable? (apart from the test, which I'd be happy to change but I don't know how for now)

apart from the test, which I'd be happy to change but I don't know how for now

I have added a context manager to move to the temporary directory and come back after the test.... @pllim is that ok with you?

Okay I've played a little with this locally and I think I got a complete solution to the issue with your test

diff --git a/astropy/io/fits/tests/test_connect.py b/astropy/io/fits/tests/test_connect.py
index 969eee2691..f2eb77a33d 100644
--- a/astropy/io/fits/tests/test_connect.py
+++ b/astropy/io/fits/tests/test_connect.py
@@ -1,6 +1,4 @@
-import contextlib
 import gc
-import os
 import warnings
 from pathlib import Path
 
@@ -60,21 +58,6 @@ def equal_data(a, b):
     return all(np.all(a[name] == b[name]) for name in a.dtype.names)
 
 
-@contextlib.contextmanager
-def working_directory(path):
-    """
-    A context manager changing the working directory to the given
-    path. The directory is changed back on exit.
-    """
-
-    cwd = os.getcwd()
-    os.chdir(path)
-    try:
-        yield
-    finally:
-        os.chdir(cwd)
-
-
 class TestSingleTable:
     def setup_class(self):
         self.data = np.array(
@@ -83,11 +66,10 @@ class TestSingleTable:
         )
 
     def test_path(self, tmp_path):
-        with working_directory(tmp_path):
-            filename = "temp.fits"
-            t1 = Table(self.data)
-            t1.write(filename, overwrite=True)
-            t1.write(Path(filename), format="fits", overwrite=True)
+        filename = tmp_path / "temp.fits"
+        t1 = Table(self.data)
+        t1.write(filename, overwrite=True)
+        t1.write(Path(filename), format="fits", overwrite=True)
 
     def test_simple(self, tmp_path):
         filename = tmp_path / "test_simple.fts"
diff --git a/astropy/io/registry/core.py b/astropy/io/registry/core.py
index b7325c4beb..3c0e1fad25 100644
--- a/astropy/io/registry/core.py
+++ b/astropy/io/registry/core.py
@@ -15,7 +15,7 @@ def _expand_user_in_args(args):
     # Conservatively attempt to apply `os.path.expanduser` to the first
     # argument, which can be either a path or the contents of a table.
     if len(args) and isinstance(args[0], PATH_TYPES):
-        ex_user = os.path.expanduser(args[0])
+        ex_user = type(args[0])(os.path.expanduser(args[0]))
         if ex_user != args[0] and os.path.exists(os.path.dirname(ex_user)):
             args = (ex_user,) + args[1:]
     return args
diff --git a/astropy/io/votable/connect.py b/astropy/io/votable/connect.py
index 8c58a9b105..68dcb63366 100644
--- a/astropy/io/votable/connect.py
+++ b/astropy/io/votable/connect.py
@@ -163,7 +163,7 @@ def write_table_votable(
         )
 
     # Check if output file already exists
-    if isinstance(output, str) and os.path.exists(output):
+    if isinstance(output, (str, os.PathLike)) and os.path.exists(output):
         if overwrite:
             os.remove(output)
         else:
diff --git a/astropy/io/votable/util.py b/astropy/io/votable/util.py
index 970a9cbd03..c8537da4f3 100644
--- a/astropy/io/votable/util.py
+++ b/astropy/io/votable/util.py
@@ -43,7 +43,7 @@ def convert_to_writable_filelike(fd, compressed=False):
     -------
     fd : :term:`file-like (writeable)`
     """
-    if isinstance(fd, str):
+    if isinstance(fd, (str, os.PathLike)):
         fd = os.path.expanduser(fd)
         if fd.endswith(".gz") or compressed:
             with gzip.GzipFile(filename=fd, mode="wb") as real_fd:

(I checked that all tests pass locally)

Re: patch -- Does this mean this PR would also add Path support to votable unified I/O if accepted?

I guess so.

We should keep this PR about the original bug.

I'm not sure these are actually separable unfortunately. My patch appears to be needed to allow this PR's test to be rewritten in an acceptable form, but my patch also doesn't work on its own: it requires this PR's change in astropy/io/fits/connect.py.

I also believe that this PR should keep focus on a specific bug.
Also, I am quite uncomfortable in keeping credit for someone else's patches.
So, please, have a look at this last version.

• the output variable is a string modified in io/registry/core.py also in the case of no directory.
• connect.py is unmodified
• tests have been added either changing directory or not using contextlib.chdir()

Supporting Path is delayed to another PR, which perhaps @neutrinoceros would like to submit soon , as the code has already been written.
@pllim , @saimn, what do you think?

Supporting Path is delayed to another PR, which perhaps @neutrinoceros would like to submit soon , as the code has already been written.

happy to submit my patch as a follow up PR if we can finish this one without it !

This time I did not forget to pull... :)
Also I'm delegating the check on the type to os.path.exists : I'm not sure what explicit type checking adds to the code.
Please, let me know if this solution is acceptable.