Skip to content
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

Merged
merged 17 commits into from
Feb 16, 2022
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 51 additions & 2 deletions src/ansiblelint/file_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ""
Expand Down Expand Up @@ -199,12 +200,60 @@ def get(self, key: Any, default: Any = None) -> Any:

@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()
if self._original_content is None:
self._original_content = self._content
return self._content

@content.setter
Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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)

def content(self, value: str) -> None:
"""Update ``content`` and calculate ``updated``."""
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():
# use self.content to populate self._original_content
_ = self.content
cognifloyd marked this conversation as resolved.
Show resolved Hide resolved
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:
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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
with open(self.path.resolve(), mode="w", encoding="utf-8") as file:
file.write(self._content or "")
self.path.resolve().write_text(self._content or "")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh. I thought resolve() returned a string, not another Path object. Yeah, this would work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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."""
return hash((self.name, self.kind))
Expand Down
192 changes: 192 additions & 0 deletions test/test_file_utils.py
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
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Why didn't you just do

Suggested change
with lintable.path.open("w", encoding="utf-8") as f:
f.write(content)
lintable.path.write_text(content)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reasoning is in #1884 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I think that you are referring to the .resolve() call, which is something that is not happening here, no?

# 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):
Copy link
Member

Choose a reason for hiding this comment

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

It is strongly recommended to always use the match= argument for all pytest.raises()/pytest.warns()/pytest.deprecated() uses to prevent accidental illegitimate passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 match= argument.

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 match= here is misleading about my intents when writing the code.

Copy link
Member

Choose a reason for hiding this comment

The 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 TypeError and the test will still pass with a bug merged in 🤷‍♂️

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