Skip to content

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

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.

Sign up for GitHub

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

Merged
taldcroft merged 1 commit into from
Sep 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .ruff.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,6 @@ lint.unfixable = [
"TRY300", # Consider `else` block
]
"astropy/io/ascii/*" = [
"PTH",
]
"astropy/io/fits/*" = [
"PTH",
Expand Down
3 changes: 2 additions & 1 deletion astropy/io/ascii/cds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import os
import re
from contextlib import suppress
from pathlib import Path

from astropy.units import Unit, UnitsWarning, UnrecognizedUnit

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

@astrofrog astrofrog Sep 7, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@neutrinoceros neutrinoceros Sep 9, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@taldcroft taldcroft Sep 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@neutrinoceros neutrinoceros Sep 9, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@dhomeier dhomeier Sep 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

self.data.table_name = os.path.basename(table)
self.data.table_name = Path(table).name

self.data.header = self.header
self.header.data = self.data
Expand Down
3 changes: 2 additions & 1 deletion astropy/io/ascii/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import warnings
from contextlib import suppress
from io import StringIO
from pathlib import Path

import numpy as np

Expand Down Expand Up @@ -1406,7 +1407,7 @@ def read(self, table):
with suppress(TypeError):
# Strings only
if os.linesep not in table + "":
self.data.table_name = os.path.basename(table)
self.data.table_name = Path(table).name

# If one of the newline chars is set as field delimiter, only
# accept the other one as line splitter
Expand Down
7 changes: 4 additions & 3 deletions astropy/io/ascii/qdp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
"""

import copy
import os
import re
import warnings
from collections.abc import Iterable
from pathlib import Path

import numpy as np

Expand Down Expand Up @@ -136,7 +138,7 @@ def _get_type_from_list_of_lines(lines, delimiter=None):
def _get_lines_from_file(qdp_file):
if "\n" in qdp_file:
lines = qdp_file.split("\n")
elif isinstance(qdp_file, str):
elif isinstance(qdp_file, (str, os.PathLike)):
with open(qdp_file) as fobj:
lines = [line.strip() for line in fobj]
elif isinstance(qdp_file, Iterable):
Expand Down Expand Up @@ -486,8 +488,7 @@ def _write_table_qdp(table, filename=None, err_specs=None):
fobj.close()

if filename is not None:
with open(filename, "w") as fobj:
print(full_string, file=fobj)
Path(filename).write_text(full_string)

return full_string.split("\n")

Expand Down
8 changes: 4 additions & 4 deletions astropy/io/ascii/setup_package.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
# Licensed under a 3-clause BSD style license

import os
from pathlib import Path

from numpy import get_include as get_numpy_include
from setuptools import Extension

ROOT = os.path.relpath(os.path.dirname(__file__))
ROOT = Path(__file__).parent.relative_to(Path.cwd())


def get_extensions():
sources = [
os.path.join(ROOT, "cparser.pyx"),
os.path.join(ROOT, "src", "tokenizer.c"),
str(ROOT / "cparser.pyx"),
str(ROOT / "src" / "tokenizer.c"),
]
ascii_ext = Extension(
name="astropy.io.ascii.cparser",
Expand Down
5 changes: 3 additions & 2 deletions astropy/io/ascii/tests/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
# Licensed under a 3-clause BSD style license - see LICENSE.rst

import os
from pathlib import Path

__all__ = [
"setup_function",
"teardown_function",
]

CWD = os.getcwd()
TEST_DIR = os.path.dirname(__file__)
CWD = Path.cwd()
TEST_DIR = Path(__file__).parent


def setup_function(function):
Expand Down
20 changes: 9 additions & 11 deletions astropy/io/ascii/ui.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import typing
import warnings
from io import StringIO
from pathlib import Path

import numpy as np

Expand Down Expand Up @@ -144,9 +145,8 @@ def _probably_html(table, maxchars=100000):
return True

# Filename ending in .htm or .html which exists
if re.search(r"\.htm[l]?$", table[-5:], re.IGNORECASE) and os.path.exists(
os.path.expanduser(table)
):
table_path = Path(table).expanduser()
if re.search(r"\.htm[l]?$", table[-5:], re.IGNORECASE) and table_path.is_file():
return True

# Table starts with HTML document type declaration
Expand Down Expand Up @@ -331,9 +331,8 @@ def _expand_user_if_path(argument):
)
if not is_str_data:
# Remain conservative in expanding the presumed-path
ex_user = os.path.expanduser(argument)
if os.path.exists(ex_user):
argument = ex_user
if (ex_user := Path(argument).expanduser()).exists():
argument = str(ex_user)
return argument


Expand Down Expand Up @@ -1006,7 +1005,7 @@ def write(
)

if isinstance(output, (str, bytes, os.PathLike)):
output = os.path.expanduser(output)
output = os.path.expanduser(output) # noqa: PTH111
if not overwrite and os.path.lexists(output):
raise OSError(NOT_OVERWRITING_MSG.format(output))

Expand Down Expand Up @@ -1059,10 +1058,9 @@ def write(
# behavior is for Python to translate \r\n (which we write because
# of os.linesep) into \r\r\n. Specifying newline='' disables any
# auto-translation.
output = open(output, "w", newline="")
output.write(outstr)
output.write(os.linesep)
output.close()
Comment on lines -1062 to -1065

@taldcroft taldcroft Sep 7, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@neutrinoceros neutrinoceros Sep 10, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

with open(output, "w", newline="") as output:
output.write(outstr)
output.write(os.linesep)
else:
output.write(outstr)
output.write(os.linesep)
Expand Down