Replaced `os.path` with `pathlib` by trag1c · Pull Request #182 · econchick/interrogate

trag1c · 2024-05-22T21:47:11Z

Hey, I just made a Pull Request!

Description

Changes all occurrences of os.path to use pathlib instead.

Motivation and Context

Closes #179

Have you tested this? If so, how?

Updated unit tests :)

Checklist for PR author(s)

Changes are covered by unit tests (no major decrease in code coverage %).
All tests pass.
Docstring coverage is 100% via tox -e docs or interrogate -c pyproject.toml (I mean, we should set a good example 😄).
Updates to documentation:
- Document any relevant additions/changes in README.rst.
- Manually update both the README.rst and docs/index.rst for any new/changed CLI flags.
- Any changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives. Find the appropriate next version in the project's __init__.py file.

Release note

NONE

for more information, see https://pre-commit.ci

trag1c · 2024-05-22T21:52:01Z

Not sure if this deserves a release note or any versionchanged directives.

I'll take a look at Windows' fails tomorrow 👍

trag1c · 2024-05-25T07:08:40Z

I'll take a look at Windows' fails tomorrow 👍

Took a bit longer but I'm getting OSErrors, seems like Windows can't install cairosvg 🤔 I feel like I'm doing something wrong here.

@econchick Could you approve the workflow? 😄

msftcangoblowm · 2025-02-18T09:18:27Z

Have been here myself.

Getting the paths to work on all platforms and only being able to test thru a gh workflow.

Here is a module that is specifically for the challenge this PR is about

https://github.com/msftcangoblowm/wreck/blob/master/src/wreck/_safe_path.py

If you hit a brick wall or feel going around in circles, this code might be helpful

Paebbels

I like that transition to pathlib, but I found many mistakes and code parts that can be improved. See my list of suggestions.

I found this PR by accident. I'm no contributor to this repository. I'm just an excessive Python user.

Paebbels · 2025-04-16T14:22:56Z

setup.py

    buf = []
    for fl in filenames:
-        with codecs.open(os.path.join(HERE, fl), "rb", encoding) as f:
+        with codecs.open(HERE / fl, "rb", encoding) as f:


Suggested change

with codecs.open(HERE / fl, "rb", encoding) as f:

with (HERE / fl).open("r", encoding) as f:

You should open the file in text mode, not binary mode and then apply the encoding. Path objects provide a Path.open() method.

Since when is the abbreviation of filename fl?

Paebbels · 2025-04-16T14:25:30Z

docs/conf.py

+    here = Path(__file__).parent.absolute()
+    with codecs.open(str(here.joinpath(*parts)), "rb", "utf-8") as f:


Suggested change

here = Path(__file__).parent.absolute()

with codecs.open(str(here.joinpath(*parts)), "rb", "utf-8") as f:

HERE = Path(__file__).parent.absolute()

with HERE.joinpath(*parts).open("r", encoding="utf-8") as f:

No need to convert to a str, almost every open function/method accepts path-like objects. Don't open a file in binary when you apply an encoding.

Other parts use HERE.

Paebbels · 2025-04-16T14:29:14Z

src/interrogate/badge_gen.py

    # need to write the badge as an svg first in order to convert it to
    # another format
-    tmp_output_file = f"{os.path.splitext(output)[0]}.tmp.svg"
+    tmp_output_file = Path(f"{output.parent / output.stem}.tmp.svg")


Suggested change

tmp_output_file = Path(f"{output.parent / output.stem}.tmp.svg")

tmp_output_file = output.with_suffix(".tmp.svg")

Source: https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.with_suffix

Paebbels · 2025-04-16T14:31:18Z

src/interrogate/badge_gen.py


    try:
-        badge = minidom.parse(output)
+        badge = minidom.parse(str(output))


Suggested change

badge = minidom.parse(str(output))

badge = minidom.parse(output)

minidom accepts file-like objects: https://docs.python.org/3/library/xml.dom.minidom.html#xml.dom.minidom.parse

Paebbels · 2025-04-16T14:34:51Z

src/interrogate/config.py

    """
-    with open(path_config, "rb") as f:
-        pyproject_toml = tomllib.load(f)
+    pyproject_toml = tomllib.loads(Path(path_config).read_text())


Suggested change

pyproject_toml = tomllib.loads(Path(path_config).read_text())

pyproject_toml = tomllib.loads(Path(path_config).read_text(encoding="utf-8"))

No encoding is specified.

Paebbels · 2025-04-16T14:58:57Z

tests/functional/test_cli.py

-
-    expected_output = expected_output.replace("\n", "")
+    expected_output_path = FIXTURES / "expected_badge.svg"
+    expected_output = expected_output_path.read_text().replace("\n", "")


Suggested change

expected_output = expected_output_path.read_text().replace("\n", "")

expected_output = expected_output_path.read_text(encoding="utf-8").replace("\n", "")

Paebbels · 2025-04-16T14:59:23Z

tests/functional/test_coverage.py

-    with open(expected_fixture) as f:
-        expected_out = f.read()
+        expected_fixture = FIXTURES / "windows" / exp_fixture_file
+    expected_out = expected_fixture.read_text()


Suggested change

expected_out = expected_fixture.read_text()

expected_out = expected_fixture.read_text(encoding="utf-8")

Paebbels · 2025-04-16T14:59:34Z

tests/functional/test_coverage.py


-    with open(expected_fixture) as f:
-        expected_out = f.read()
+    expected_out = expected_fixture.read_text()


Suggested change

expected_out = expected_fixture.read_text()

expected_out = expected_fixture.read_text(encoding="utf-8")

Paebbels · 2025-04-16T15:00:07Z

tests/unit/test_badge_gen.py

-        expected = f.read()
-        expected = expected.replace("\n", "").replace("\r", "")
+    expected_fixture = FIXTURES / "default-style" / "99.svg"
+    expected = expected_fixture.read_text().replace("\n", "").replace("\r", "")


Suggested change

expected = expected_fixture.read_text().replace("\n", "").replace("\r", "")

expected = expected_fixture.read_text(encoding="utf-8").replace("\n", "").replace("\r", "")

Paebbels · 2025-04-16T15:00:53Z

tests/unit/test_badge_gen.py

+    expected_contents = (
+        expected_fixture.read_bytes()
+        if out_format == "png"
+        else expected_fixture.read_text()


Suggested change

else expected_fixture.read_text()

else expected_fixture.read_text(encoding="utf-8")

trag1c · 2025-04-21T09:16:04Z

I like that transition to pathlib, but I found many mistakes and code parts that can be improved. See my list of suggestions.

I found this PR by accident. I'm no contributor to this repository. I'm just an excessive Python user.

Thanks, a lot of these are good suggestions!

Not sure what's up with the snark though (e.g. "Since when is the abbreviation of filename fl?"), I didn't write the original code, just touching up the os.path bits :)

I'll probably hold off on applying these until there's some interest from Lynn in this PR actually going somewhere.

Paebbels · 2025-04-21T09:19:42Z

@trag1c The comment about fl was not meant for you, just for the maintainer when checking this review :)

trag1c added 8 commits May 22, 2024 14:48

Updated docs conf to use pathlib

cd06660

Updated setup.py to use pathlib

257428c

Made badge_gen use pathlib

b275221

Made cli use pathlib

7cbdd3b

Made config use pathlib

c5ab0e8

Replaced os.path with pathlib in misc files

a026063

Made most of coverage use pathlib

3b3c764

Made remaining coverage code use pathlib

98b9501

trag1c marked this pull request as ready for review May 22, 2024 21:48

[pre-commit.ci] auto fixes from pre-commit.com hooks

90c9dd8

for more information, see https://pre-commit.ci

trag1c added 2 commits May 22, 2024 23:54

Removed unused import

ab493c8

Made mypy happy (hopefully)

6c6d020

trag1c changed the title ~~Replaced os.path with `pathlib~~ Replaced os.path with pathlib May 22, 2024

Paebbels suggested changes Apr 16, 2025

View reviewed changes

	with codecs.open(HERE / fl, "rb", encoding) as f:
	with (HERE / fl).open("r", encoding) as f:

		here = Path(__file__).parent.absolute()
		with codecs.open(str(here.joinpath(*parts)), "rb", "utf-8") as f:

	tmp_output_file = Path(f"{output.parent / output.stem}.tmp.svg")
	tmp_output_file = output.with_suffix(".tmp.svg")

	badge = minidom.parse(str(output))
	badge = minidom.parse(output)

	pyproject_toml = tomllib.loads(Path(path_config).read_text())
	pyproject_toml = tomllib.loads(Path(path_config).read_text(encoding="utf-8"))

	expected_output = expected_output_path.read_text().replace("\n", "")
	expected_output = expected_output_path.read_text(encoding="utf-8").replace("\n", "")

	expected_out = expected_fixture.read_text()
	expected_out = expected_fixture.read_text(encoding="utf-8")

	expected = expected_fixture.read_text().replace("\n", "").replace("\r", "")
	expected = expected_fixture.read_text(encoding="utf-8").replace("\n", "").replace("\r", "")

	else expected_fixture.read_text()
	else expected_fixture.read_text(encoding="utf-8")

Uh oh!

Conversation

trag1c commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Hey, I just made a Pull Request!

Description

Motivation and Context

Have you tested this? If so, how?

Checklist for PR author(s)

Release note

Uh oh!

trag1c commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trag1c commented May 25, 2024

Uh oh!

msftcangoblowm commented Feb 18, 2025

Uh oh!

Paebbels 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!

trag1c commented Apr 21, 2025

Uh oh!

Paebbels commented Apr 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

trag1c commented May 22, 2024 •

edited

Loading

trag1c commented May 22, 2024 •

edited

Loading