-
Notifications
You must be signed in to change notification settings - Fork 636
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
Enable Lintables to be modified #1884
Changes from all commits
63aad50
0e8049e
01d44b5
9cbc886
398046b
a984efb
415a24f
e79e44a
784fe7a
ba76a0f
35b0c7d
06147ec
d87a6d3
9482195
f544f32
d98cf31
88da8d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -10,7 +10,7 @@ | |||||||
from contextlib import contextmanager | ||||||||
from pathlib import Path | ||||||||
from tempfile import NamedTemporaryFile | ||||||||
from typing import TYPE_CHECKING, Any, Dict, Iterator, List, Optional, Set, Union | ||||||||
from typing import TYPE_CHECKING, Any, Dict, Iterator, List, Optional, Set, Union, cast | ||||||||
|
||||||||
# import wcmatch | ||||||||
import wcmatch.pathlib | ||||||||
|
@@ -147,7 +147,8 @@ def __init__( | |||||||
else: | ||||||||
self.name = str(name) | ||||||||
self.path = name | ||||||||
self._content = content | ||||||||
self._content = self._original_content = content | ||||||||
self.updated = False | ||||||||
|
||||||||
# if the lintable is part of a role, we save role folder name | ||||||||
self.role = "" | ||||||||
|
@@ -197,13 +198,67 @@ def get(self, key: Any, default: Any = None) -> Any: | |||||||
except NotImplementedError: | ||||||||
return default | ||||||||
|
||||||||
def _populate_content_cache_from_disk(self) -> None: | ||||||||
with open(self.path.resolve(), mode="r", encoding="utf-8") as f: | ||||||||
self._content = f.read() | ||||||||
if self._original_content is None: | ||||||||
self._original_content = self._content | ||||||||
|
||||||||
@property | ||||||||
def content(self) -> str: | ||||||||
"""Retried file content, from internal cache or disk.""" | ||||||||
"""Retrieve file content, from internal cache or disk.""" | ||||||||
if self._content is None: | ||||||||
with open(self.path.resolve(), mode="r", encoding="utf-8") as f: | ||||||||
self._content = f.read() | ||||||||
return self._content | ||||||||
self._populate_content_cache_from_disk() | ||||||||
return cast(str, self._content) | ||||||||
|
||||||||
@content.setter | ||||||||
def content(self, value: str) -> None: | ||||||||
"""Update ``content`` and calculate ``updated``. | ||||||||
|
||||||||
To calculate ``updated`` this will read the file from disk if the cache | ||||||||
has not already been populated. | ||||||||
""" | ||||||||
if not isinstance(value, str): | ||||||||
raise TypeError(f"Expected str but got {type(value)}") | ||||||||
if self._original_content is None: | ||||||||
if self._content is not None: | ||||||||
self._original_content = self._content | ||||||||
elif self.path.exists(): | ||||||||
self._populate_content_cache_from_disk() | ||||||||
else: | ||||||||
# new file | ||||||||
self._original_content = "" | ||||||||
self.updated = self._original_content != value | ||||||||
self._content = value | ||||||||
|
||||||||
@content.deleter | ||||||||
def content(self) -> None: | ||||||||
"""Reset the internal content cache.""" | ||||||||
self._content = None | ||||||||
|
||||||||
def write(self, force: bool = False) -> None: | ||||||||
"""Write the value of ``Lintable.content`` to disk. | ||||||||
|
||||||||
This only writes to disk if the content has been updated (``Lintable.updated``). | ||||||||
For example, you can update the content, and then write it to disk like this: | ||||||||
|
||||||||
.. code:: python | ||||||||
|
||||||||
lintable.content = new_content | ||||||||
lintable.write() | ||||||||
|
||||||||
Use ``force=True`` when you want to force a content rewrite even if the | ||||||||
content has not changed. For example: | ||||||||
|
||||||||
.. code:: python | ||||||||
|
||||||||
lintable.write(force=True) | ||||||||
""" | ||||||||
if not force and not self.updated: | ||||||||
# No changes to write. | ||||||||
return | ||||||||
with open(self.path.resolve(), mode="w", encoding="utf-8") as file: | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why use an old-fashioned low-level API for this and not pathlib-native one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a file is a symlink, I don't want to replace the symlink with an actual file, I want to update the contents of the symlink's target. I do not see pathlib methods for open or write that follow symlinks. This matches the implementaiton introduced in #1770. |
||||||||
file.write(self._content or "") | ||||||||
Comment on lines
+260
to
+261
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I didn't mean to suggest removing the resolution. I was thinking about something like
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh. I thought There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #1913 |
||||||||
|
||||||||
def __hash__(self) -> int: | ||||||||
"""Return a hash value of the lintables.""" | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,5 +1,6 @@ | ||||||||
"""Tests for file utility functions.""" | ||||||||
import os | ||||||||
import time | ||||||||
from argparse import Namespace | ||||||||
from pathlib import Path | ||||||||
from typing import Any, Dict, Union | ||||||||
|
@@ -212,3 +213,194 @@ def test_guess_project_dir(tmp_path: Path) -> None: | |||||||
with cwd(str(tmp_path)): | ||||||||
result = guess_project_dir(None) | ||||||||
assert result == str(tmp_path) | ||||||||
|
||||||||
|
||||||||
BASIC_PLAYBOOK = """ | ||||||||
- name: "playbook" | ||||||||
tasks: | ||||||||
- name: hello | ||||||||
debug: | ||||||||
msg: 'world' | ||||||||
""" | ||||||||
|
||||||||
|
||||||||
@pytest.fixture | ||||||||
def tmp_updated_lintable( | ||||||||
tmp_path: Path, path: str, content: str, updated_content: str | ||||||||
) -> Lintable: | ||||||||
"""Create a temp file Lintable with a content update that is not on disk.""" | ||||||||
lintable = Lintable(tmp_path / path, content) | ||||||||
with lintable.path.open("w", encoding="utf-8") as f: | ||||||||
f.write(content) | ||||||||
Comment on lines
+233
to
+234
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why didn't you just do
Suggested change
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reasoning is in #1884 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that you are referring to the |
||||||||
# move mtime to a time in the past to avoid race conditions in the test | ||||||||
mtime = time.time() - 60 * 60 # 1hr ago | ||||||||
os.utime(str(lintable.path), (mtime, mtime)) | ||||||||
lintable.content = updated_content | ||||||||
return lintable | ||||||||
|
||||||||
|
||||||||
@pytest.mark.parametrize( | ||||||||
("path", "content", "updated_content", "updated"), | ||||||||
( | ||||||||
pytest.param( | ||||||||
"no_change.yaml", BASIC_PLAYBOOK, BASIC_PLAYBOOK, False, id="no_change" | ||||||||
), | ||||||||
pytest.param( | ||||||||
"quotes.yaml", | ||||||||
BASIC_PLAYBOOK, | ||||||||
BASIC_PLAYBOOK.replace('"', "'"), | ||||||||
True, | ||||||||
id="updated_quotes", | ||||||||
), | ||||||||
pytest.param( | ||||||||
"shorten.yaml", BASIC_PLAYBOOK, "# short file\n", True, id="shorten_file" | ||||||||
), | ||||||||
), | ||||||||
) | ||||||||
def test_lintable_updated( | ||||||||
path: str, content: str, updated_content: str, updated: bool | ||||||||
) -> None: | ||||||||
"""Validate ``Lintable.updated`` when setting ``Lintable.content``.""" | ||||||||
lintable = Lintable(path, content) | ||||||||
|
||||||||
assert lintable.content == content | ||||||||
|
||||||||
lintable.content = updated_content | ||||||||
|
||||||||
assert lintable.content == updated_content | ||||||||
|
||||||||
assert lintable.updated is updated | ||||||||
|
||||||||
|
||||||||
@pytest.mark.parametrize( | ||||||||
"updated_content", ((None,), (b"bytes",)), ids=("none", "bytes") | ||||||||
) | ||||||||
def test_lintable_content_setter_with_bad_types(updated_content: Any) -> None: | ||||||||
"""Validate ``Lintable.updated`` when setting ``Lintable.content``.""" | ||||||||
lintable = Lintable("bad_type.yaml", BASIC_PLAYBOOK) | ||||||||
assert lintable.content == BASIC_PLAYBOOK | ||||||||
|
||||||||
with pytest.raises(TypeError): | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is strongly recommended to always use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doing that feels like relying on implementation details. Had I done that with the old implementation, I would have had to modify it with this implementation because the TypeError is now being raised in my code, but something else rose it with the old implementation. So, I don't like using the This test is not checking for a message. It is checking that it raises a TypeError for non-str things. I don't care why or what message is used, so adding a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Imagine you refactor that code and make a mistake in a function signature you call inside of the setter or down the stack. It will cause a |
||||||||
lintable.content = updated_content | ||||||||
|
||||||||
assert not lintable.updated | ||||||||
|
||||||||
|
||||||||
def test_lintable_with_new_file(tmp_path: Path) -> None: | ||||||||
"""Validate ``Lintable.updated`` for a new file.""" | ||||||||
lintable = Lintable(tmp_path / "new.yaml") | ||||||||
|
||||||||
with pytest.raises(FileNotFoundError): | ||||||||
_ = lintable.content | ||||||||
|
||||||||
lintable.content = BASIC_PLAYBOOK | ||||||||
assert lintable.content == BASIC_PLAYBOOK | ||||||||
|
||||||||
assert lintable.updated | ||||||||
|
||||||||
assert not lintable.path.exists() | ||||||||
lintable.write() | ||||||||
assert lintable.path.exists() | ||||||||
assert lintable.path.read_text(encoding="utf-8") == BASIC_PLAYBOOK | ||||||||
|
||||||||
|
||||||||
@pytest.mark.parametrize( | ||||||||
("path", "force", "content", "updated_content", "updated"), | ||||||||
( | ||||||||
pytest.param( | ||||||||
"no_change.yaml", | ||||||||
False, | ||||||||
BASIC_PLAYBOOK, | ||||||||
BASIC_PLAYBOOK, | ||||||||
False, | ||||||||
id="no_change", | ||||||||
), | ||||||||
pytest.param( | ||||||||
"forced.yaml", | ||||||||
True, | ||||||||
BASIC_PLAYBOOK, | ||||||||
BASIC_PLAYBOOK, | ||||||||
False, | ||||||||
id="forced_rewrite", | ||||||||
), | ||||||||
pytest.param( | ||||||||
"quotes.yaml", | ||||||||
False, | ||||||||
BASIC_PLAYBOOK, | ||||||||
BASIC_PLAYBOOK.replace('"', "'"), | ||||||||
True, | ||||||||
id="updated_quotes", | ||||||||
), | ||||||||
pytest.param( | ||||||||
"shorten.yaml", | ||||||||
False, | ||||||||
BASIC_PLAYBOOK, | ||||||||
"# short file\n", | ||||||||
True, | ||||||||
id="shorten_file", | ||||||||
), | ||||||||
pytest.param( | ||||||||
"forced.yaml", | ||||||||
True, | ||||||||
BASIC_PLAYBOOK, | ||||||||
BASIC_PLAYBOOK.replace('"', "'"), | ||||||||
True, | ||||||||
id="forced_and_updated", | ||||||||
), | ||||||||
), | ||||||||
) | ||||||||
def test_lintable_write( | ||||||||
tmp_updated_lintable: Lintable, | ||||||||
force: bool, | ||||||||
content: str, | ||||||||
updated_content: str, | ||||||||
updated: bool, | ||||||||
) -> None: | ||||||||
"""Validate ``Lintable.write`` writes when it should.""" | ||||||||
pre_updated = tmp_updated_lintable.updated | ||||||||
pre_stat = tmp_updated_lintable.path.stat() | ||||||||
|
||||||||
tmp_updated_lintable.write(force=force) | ||||||||
|
||||||||
post_stat = tmp_updated_lintable.path.stat() | ||||||||
post_updated = tmp_updated_lintable.updated | ||||||||
|
||||||||
# write() should not hide that an update happened | ||||||||
assert pre_updated == post_updated == updated | ||||||||
|
||||||||
if force or updated: | ||||||||
assert pre_stat.st_mtime < post_stat.st_mtime | ||||||||
else: | ||||||||
assert pre_stat.st_mtime == post_stat.st_mtime | ||||||||
|
||||||||
with tmp_updated_lintable.path.open("r", encoding="utf-8") as f: | ||||||||
post_content = f.read() | ||||||||
|
||||||||
if updated: | ||||||||
assert content != post_content | ||||||||
else: | ||||||||
assert content == post_content | ||||||||
assert post_content == updated_content | ||||||||
|
||||||||
|
||||||||
@pytest.mark.parametrize( | ||||||||
("path", "content", "updated_content"), | ||||||||
( | ||||||||
pytest.param( | ||||||||
"quotes.yaml", | ||||||||
BASIC_PLAYBOOK, | ||||||||
BASIC_PLAYBOOK.replace('"', "'"), | ||||||||
id="updated_quotes", | ||||||||
), | ||||||||
), | ||||||||
) | ||||||||
def test_lintable_content_deleter( | ||||||||
tmp_updated_lintable: Lintable, | ||||||||
content: str, | ||||||||
updated_content: str, | ||||||||
) -> None: | ||||||||
"""Ensure that resetting content cache triggers re-reading file.""" | ||||||||
assert content != updated_content | ||||||||
assert tmp_updated_lintable.content == updated_content | ||||||||
del tmp_updated_lintable.content | ||||||||
assert tmp_updated_lintable.content == content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of a setter seems weird to me here. A natural goal of setters is to set values mostly but this one also does I/O and shuffles the internal state. It feels like this sort of thing would require an explicit method call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTOH the whole idea of having a mutable object appears questionable — would it be possible to have some transformation API that would take one lintable and construct/return a new one in an immutable manner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lintable instance is used all over the place. Making it immutable feels very odd. I would have to somehow make everything that accesses content via a Lintable check to see if something else has an in-memory change queued up to write to the file.
Feel free to open an alternative PR that shows how such an immutable object might work. I'm not willing to pursue that avenue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to an alternative to the setter, but I'm tired of reimplementing this. Show me what interface you want instead of a setter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it may be hard to make it immutable with the current codebase, I won't insist on refactoring the whole project, of course :)
Have you considered exposing a method? (just thinking out loud)