MNT: Run PTH flake test in prep for supporting pathlib (io.ascii) by neutrinoceros · Pull Request #16954 · astropy/astropy

neutrinoceros · 2024-09-06T08:35:25Z

Description

Ref #16924
This is in the continuation of #16060

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-09-06T08:35:51Z

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.

github-actions · 2024-09-06T08:35:53Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

pllim · 2024-09-06T18:01:01Z

astropy/io/ascii/ui.py

-        output.write(outstr)
-        output.write(os.linesep)
-        output.close()
+        if not output:


But there is already this check above:

if output is None: output = sys.stdout

This check is different: it also invalidates the empty string '' because Path('') means "the current work directory", and this scenario (passing output='' and expecting a FileNotFoundError) is actually exercised in a test.

What was the old behavior for output=''? Does this need a change log?

Au contraire, this is being strictly backward compatible: open('') raises the same exception.

I am seeing this error message:

>>> open("", "w") FileNotFoundError: [Errno 2] No such file or directory: ''

I'm going to revert this and simply refactor with a context manager as suggested in https://github.com/astropy/astropy/pull/16954/files#r1748045678

taldcroft

Looks OK but with requested changes. Overall in the Path migration I think you should mostly avoid doing a blind replacement of open(...) with Path().read/write_text().

There is nothing wrong with open() and the two cases here show that open() is often preferable because it is a streaming interface.

taldcroft · 2024-09-07T10:21:37Z

astropy/io/ascii/qdp.py

+    elif isinstance(qdp_file, (str, os.PathLike)):
+        lines = [line.strip() for line in Path(qdp_file).read_text().splitlines()]


This new code ends up making two additional copies of the entire file in memory, one the full read_text() output and second the splitlines() output. In general splitlines() is not a drop-in replacement for iterating over open() since splitlines() removes the line endings. In this case it doesn't matter but best to keep this Path refactoring as close to the original code as possible.

So basically the only change here should be isinstance(qdp_file, (str, os.PathLike)):.

In this case it doesn't matter but best to keep this Path refactoring as close to the original code as possible.

100% agreed. Any behavioral difference with the original code that I don't advertise myself in a comment is a mistake from me, so thank you for your attention to details !

taldcroft · 2024-09-07T10:38:32Z

astropy/io/ascii/ui.py

-        output.write(outstr)
-        output.write(os.linesep)
-        output.close()
+        if not output:


I am seeing this error message:

>>> open("", "w") FileNotFoundError: [Errno 2] No such file or directory: ''

taldcroft · 2024-09-07T10:50:56Z

astropy/io/ascii/ui.py

-    if isinstance(output, (str, bytes, os.PathLike)):
-        output = os.path.expanduser(output)
+    if isinstance(output, (str, bytes, os.PathLike)) and output:
+        output = Path(output).expanduser()


I would prefer to revert the changes In this function and stick with the original os.path.expanduser(). In this case using Path is introducing new complexity to the logic because of the Path("") => cwd() behavior. So maybe add a #noqa ....

taldcroft · 2024-09-07T10:51:53Z

astropy/io/ascii/ui.py

+            # Path("") gives the current working directory
+            raise FileNotFoundError("No such file or directory")
+        Path(output).write_text(
+            outstr + os.linesep,


This creates an unnecessary temporary string outstr + os.linesep.

taldcroft · 2024-09-07T10:52:53Z

astropy/io/ascii/ui.py

-        output = open(output, "w", newline="")
-        output.write(outstr)
-        output.write(os.linesep)
-        output.close()


This should just be changed to an open(...) context manager.

astrofrog · 2024-09-07T15:55:29Z

astropy/io/ascii/cds.py

@@ -379,7 +380,7 @@ def read(self, table):
            with suppress(TypeError):
                # For strings only
                if os.linesep not in table + "":


@taldcroft just out of pure curiosity, what is the purpose of the + "" here?

I assume this is to trigger a TypeError if table isn't a string ?

Exactly what @neutrinoceros said. This is a very old idiom to check for string-like (somewhat like a protocol, asking if it supports appending a str) instead of an explicit str type check.

What's the best way to check for string-like? Or does isinstance(table, str) cover all realistic use cases?

isinstance is definitely more idiomatic here; I can't think of any type other than str that accepts another str in __add__

From BaseReader.read in core.py below, which uses the same construct, the only other input cases are file-like or list of strings (the table lines), so they should all be covered.

neutrinoceros · 2024-09-09T09:11:53Z

Overall in the Path migration I think you should mostly avoid doing a blind replacement of open(...) with Path().read/write_text().

There is nothing wrong with open() and the two cases here show that open() is often preferable because it is a streaming interface.

@taldcroft I agree. I'll open a PR to propose ignoring that specific rule (PTH123) more permanently. Moving this one to draft until it is resolved.

neutrinoceros · 2024-09-10T07:10:18Z

I think I've now addressed all comments from @taldcroft, let me know if there's anything I missed !

taldcroft · 2024-09-12T10:33:57Z

astropy/io/ascii/ui.py

+        with open(output, "w", newline="") as output:
+            output.write(outstr)
+            output.write(os.linesep)
+            output.close()


The .close() is not needed with the context manager.

Thanks for catching this. Fixed.

…tropy#16954)

github-actions bot added testing io.ascii dev-automation labels Sep 6, 2024

neutrinoceros force-pushed the io.ascii/rfc/pth_checks branch from e4b63bd to 3412167 Compare September 6, 2024 08:38

neutrinoceros mentioned this pull request Sep 6, 2024

MNT: Run PTH flake test in prep for supporting pathlib #16924

Closed

neutrinoceros force-pushed the io.ascii/rfc/pth_checks branch from 3412167 to 48210e4 Compare September 6, 2024 09:28

neutrinoceros marked this pull request as ready for review September 6, 2024 09:40

neutrinoceros requested review from WilliamJamieson, dhomeier, nstarman and taldcroft as code owners September 6, 2024 09:40

pllim added this to the v7.0.0 milestone Sep 6, 2024

pllim reviewed Sep 6, 2024

View reviewed changes

pllim added no-changelog-entry-needed Extra CI Run cron CI as part of PR labels Sep 6, 2024

taldcroft requested changes Sep 7, 2024

View reviewed changes

astrofrog reviewed Sep 7, 2024

View reviewed changes

neutrinoceros marked this pull request as draft September 9, 2024 09:16

neutrinoceros mentioned this pull request Sep 9, 2024

STY: permanently ignore ruff rule PTH123 (builtin-open) #16972

Merged

1 task

neutrinoceros changed the title ~~MNT: Run PTH flake test in prep for supporting pathlib (io.ascii)~~ MNT: Run PTH flake test in prep for supporting pathlib (io.ascii) (⏰ wait for #16972) Sep 9, 2024

neutrinoceros force-pushed the io.ascii/rfc/pth_checks branch from 48210e4 to f897605 Compare September 10, 2024 07:09

neutrinoceros changed the title ~~MNT: Run PTH flake test in prep for supporting pathlib (io.ascii) (⏰ wait for #16972)~~ MNT: Run PTH flake test in prep for supporting pathlib (io.ascii) Sep 10, 2024

neutrinoceros marked this pull request as ready for review September 10, 2024 07:10

taldcroft reviewed Sep 12, 2024

View reviewed changes

MNT: Run PTH flake test in prep for supporting pathlib (io.ascii)

e04f9b3

neutrinoceros force-pushed the io.ascii/rfc/pth_checks branch from f897605 to e04f9b3 Compare September 12, 2024 10:36

taldcroft approved these changes Sep 12, 2024

View reviewed changes

taldcroft enabled auto-merge (squash) September 12, 2024 10:41

taldcroft merged commit 3bf33f6 into astropy:main Sep 12, 2024

neutrinoceros deleted the io.ascii/rfc/pth_checks branch September 12, 2024 11:03

This was referenced Dec 19, 2024

BUG: wrongly assumed Path when parsing html #17562

Closed

BUG: Fix parsing ASCII table when data starts with tilda #17564

Closed

kkrutkowski pushed a commit to kkrutkowski/astropy that referenced this pull request Apr 13, 2025

MNT: Run PTH flake test in prep for supporting pathlib (io.ascii) (as…

b7978f7

…tropy#16954)

neutrinoceros mentioned this pull request Jun 10, 2025

BLD: fix building from source on Windows from the top of the repository #18253

Merged

1 task

		elif isinstance(qdp_file, (str, os.PathLike)):
		lines = [line.strip() for line in Path(qdp_file).read_text().splitlines()]

Uh oh!

Conversation

neutrinoceros commented Sep 6, 2024

Description

Uh oh!

github-actions bot commented Sep 6, 2024

Uh oh!

github-actions bot commented Sep 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhomeier Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Sep 9, 2024

Uh oh!

neutrinoceros commented Sep 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

neutrinoceros Sep 6, 2024 •

edited

Loading

dhomeier Sep 9, 2024 •

edited

Loading