Enable Lintables to be modified #1884

cognifloyd · 2022-02-13T01:20:29Z

We need a way to write files and keep track of how we've changed them.

This prepares for formatting in transforms in #1828. Feedback from that PR suggests that we'll need at least a count of the files that have changed and possibly a diff view that shows how files have changed.

This adds a setter for Lintable.content which monitors the content for changes..
When the content is changed, it sets Lintable.updated = True.
Original content is also preserved to facilitate further comparison or resetting the content if needed.

There is also a deleter for Lintable.content so that if something needs to modify a file externally, the developer can trigger reloading the content from disk with del lintable.content.

NB: I originally used a stream/file-like object because that's what ruamel.yaml's API expects. But that made the interface more confusing. The Transformer will have to create a StringIO instance on the fly when using ruamel.yaml.

Replaces #1865 (an alternative implementation)

webknjaz · 2022-02-13T11:28:18Z

Where are these streams closed? If they aren't, this will cause ResourceWarnings.

cognifloyd · 2022-02-13T16:29:46Z

The current implementation relies on StringIO so closing them is not an issue. If we need to switch to temporary files then we'd probably need to auto open/close them or something. I'm not sure where.

lilatomic · 2022-02-14T02:40:39Z

At least to me, it would be clearer to rename Lintable._content to original_content, and Lintable._writable to Lintable._content. That's the spirit of the comments next to them, and I think it would help remove confusion like "Why do we never update the content?". I would also in Lintable.__init__ assign both of them the value of content, I think that would eliminate a few null checks.

Could you also provide some guidance on when we would use content vs writable? That is, if I have a rule that can rewrite, would I always request a writable? or would I request the content, see if I need to change it, and then request the writable? I noticed that the tests seem to truncate the contents of the writable, I'm not sure if that's illustrative of the expected usage.

Could you also talk about why you went with a method to expose a writable stream, rather than a method to set the updated content? I understand why we're using streams over string; but why have def writable(self) -> TextIO rather than def update(self, contents: TextIO) -> None? I think an advantage of an update method is that it would allow for linters and lintables to better track their updates, eg with def update(self, contents: TextIO, linter_class: Type[AnsibleLintRule], comment: str = "") -> None. I think this would also simplify the usage of content vs writable, since a rule would always use content for the content and only invoke update if it needed to do that.

I'm excited about rules being able to remediate Ansible, and I think this lays important groundwork for that feature.

cognifloyd · 2022-02-14T17:34:54Z

OK. I changed the implementation from a Lintable.writable stream to a Lintable.content setter. I kept the same tests (modifying them to use Lintable.content instead of Lintable.writable).

The Lintable could represent a new file or it could be updating an existing file. Either way, this allows us to detect file updates. And it allows us (in the future) to provide some kind of diff-style output for those changes.

src/ansiblelint/file_utils.py

webknjaz · 2022-02-14T22:58:27Z

Why are the comments minimized?

cognifloyd · 2022-02-15T02:31:01Z

Why are the comments minimized?

Because they were about the old implementation. You asked about streams, for example, which this does not use any more. So I marked the comments as outdated.

webknjaz · 2022-02-15T05:39:56Z

I don't think we need to do this with legitimate comments. It's harder to find them this way. We mostly hide spam / outdated bot comments but doing this doesn't generate any notifications decreasing the chances of finding out what happened and why. So it's also bad for transparency. If the conversations need to be disregarded, just leave a comment about that — it's probably the best way to keep the discussions/decisions traceable.

Besides, GitHub already collapses big chunk of comments anyway. So when I open the thread to see the updates/answers to previous questions, it's rather confusing to see some approvals combined with "no reply" and "removed questions". Looks like this adds to the cognitive load of people trying to find own discussions that they started.

cognifloyd · 2022-02-15T06:11:19Z

okey dokey. unhid

ssbarnea · 2022-02-15T06:53:43Z

I do mark comments as outdated every-time they no longer apply to current implementation, just to keep the review size easy to read to other reviewers, including myself. The moment they become outdated, they become spam, and that is why github added these particular reasons when you hide them.

AFAIK, @webknjaz is the only one that was intrigued by this. Maybe it is time to check with others from the team and see what think.

I personally find long conversations hard to follow, hide option helps fighting that.

webknjaz · 2022-02-15T17:12:27Z

Well, if there's disregarded comments without replies for no reason, it doesn't look good. I wasn't intrigued, I was only shocked by the disrespect. There is no indication informing me that everything has been updated and needs rechecking. The only indication is that there are some new commits without any context explaining that they contain different content. Anyway, I don't see why you disregard what I said — I merely described what signals I was getting and what I saw with my own eyes. I don't think you know better what I saw unless I explain, which I did.

webknjaz · 2022-02-15T17:20:25Z

okey dokey. unhid

@cognifloyd thanks! For the future — it may be fine to hide things but then, take the responsibility of notifying the affected parties about this with an explicit comment and mentions that would trigger notifications and give people a chance to see what happened without forcing them to guess whether our judgment about their comments was reasonable. We can't always know upfront if what you thought another person meant is the same thing you perceived. Besides, we don't know/can't assume if the other party is caught up with the updates and making things disappear would inevitably cause extra cognitive load for those who care to understand how the context they may remember got transformed into a new one.

webknjaz · 2022-02-15T17:37:34Z

test/test_file_utils.py

+    lintable = Lintable("bad_type.yaml", BASIC_PLAYBOOK)
+    assert lintable.content == BASIC_PLAYBOOK
+
+    with pytest.raises(TypeError):


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

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.

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 🤷‍♂️

webknjaz · 2022-02-15T17:39:57Z

test/test_file_utils.py

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


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)

?

Reasoning is in #1884 (comment)

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

webknjaz · 2022-02-15T17:43:23Z

src/ansiblelint/file_utils.py

+        if not force and not self.updated:
+            # No changes to write.
+            return
+        with open(self.path.resolve(), mode="w", encoding="utf-8") as file:


Why use an old-fashioned low-level API for this and not pathlib-native one?

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.

webknjaz · 2022-02-15T17:48:25Z

src/ansiblelint/file_utils.py

        return self._content

+    @content.setter


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.

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?

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.

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.

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)

src/ansiblelint/file_utils.py

ssbarnea · 2022-02-15T19:20:28Z

@cidrblock Can you please help with this one?

cidrblock

Looks good.

The only comment I have, which is totally subjective and could go in this PR, in another PR or in the trash.......... and it's probably because I'm less familiar with the patterns in the repo.

I (personally) prefer to see all the class self.* attributes defined and typed at the top of the init, even without default values. I tend to look there first to see what something is when used later and it's type.... especially when I'm seeing code for the first time. Your call.

cognifloyd · 2022-02-15T23:12:32Z

The only comment I have, which is totally subjective and could go in this PR, in another PR [snip]

I (personally) prefer to see all the class self.* attributes defined and typed at the top of the init, even without default values.

I think that would be a fine follow-up PR. I'm trying to minimize the refactoring in this PR.

ssbarnea · 2022-02-16T08:50:22Z

As none of the current opened comments is critical, I will merge it as is. We can address these in follow-ups, especially as it is likely that we would need to tune the implementation while we start using it.

webknjaz

Apparently some of the comments were not posted yesterday. Submitting them now.

webknjaz · 2022-02-15T19:54:11Z

src/ansiblelint/file_utils.py

+        with open(self.path.resolve(), mode="w", encoding="utf-8") as file:
+            file.write(self._content or "")


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 "")

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

webknjaz · 2022-02-15T19:55:40Z

test/test_file_utils.py

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


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

webknjaz · 2022-02-16T16:22:02Z

test/test_file_utils.py

+    lintable = Lintable("bad_type.yaml", BASIC_PLAYBOOK)
+    assert lintable.content == BASIC_PLAYBOOK
+
+    with pytest.raises(TypeError):


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 🤷‍♂️

cognifloyd requested a review from a team as a code owner February 13, 2022 01:20

cognifloyd requested review from relrod, ssbarnea and ganeshrn and removed request for a team February 13, 2022 01:20

cognifloyd force-pushed the writables branch from d9d963c to b7b7b9d Compare February 13, 2022 01:24

cognifloyd added the feature label Feb 13, 2022

cognifloyd self-assigned this Feb 13, 2022

cognifloyd mentioned this pull request Feb 13, 2022

Add Lintable.open() a change detecting wrapper around Path.open() #1865

Closed

ssbarnea mentioned this pull request Feb 13, 2022

Remove outdated ruamel-yaml version constraints #1889

Merged

cognifloyd changed the title ~~Add Lintable.writable stream and Lintable.updated indicator~~ Allow setting Lintable.content and add Lintable.updated indicator Feb 14, 2022

cognifloyd added 9 commits February 14, 2022 11:38

Add Lintable.writable stream to handle updating files

63aad50

The Lintable could represent a new file or it could be updating an existing file. Either way, this allows us to detect file updates. And it allows us (in the future) to provide some kind of diff-style output for those changes.

Add test for Lintable.updated

0e8049e

Split writable test into more distinct tests

01d44b5

Add tests for Lintable.write

9cbc886

Cleanup Lintable tests

398046b

Ensure Lintable.write() does not changed Lintable.updated

a984efb

Fix test mtime race condition

415a24f

add Lintable.content setter to replace writable

e79e44a

Drop Lintable.writable

784fe7a

cognifloyd force-pushed the writables branch from 989b8e4 to 784fe7a Compare February 14, 2022 17:38

cognifloyd mentioned this pull request Feb 14, 2022

New Feature: Reformat YAML files with ruamel.yaml #1828

Closed

2 tasks

cognifloyd added 2 commits February 14, 2022 12:14

Drop more Lintable.writable references

ba76a0f

Trigger re-reading Lintable by resetting content cache

35b0c7d

cognifloyd changed the title ~~Allow setting Lintable.content and add Lintable.updated indicator~~ Add Lintable.content setter and deleter + add Lintable.updated indicator Feb 14, 2022

ssbarnea requested a review from webknjaz February 14, 2022 19:14

Use _original_content != value to calculate Lintable.updated

d87a6d3

cognifloyd commented Feb 14, 2022

View reviewed changes

src/ansiblelint/file_utils.py Outdated Show resolved Hide resolved

Load original_content from disk if needed to calculate updated

9482195

webknjaz reviewed Feb 15, 2022

View reviewed changes

src/ansiblelint/file_utils.py Outdated Show resolved Hide resolved

Use a method to populate lintable content cache

f544f32

cognifloyd force-pushed the writables branch from 2909b21 to f544f32 Compare February 15, 2022 18:31

cidrblock self-requested a review February 15, 2022 19:29

cidrblock approved these changes Feb 15, 2022

View reviewed changes

ssbarnea added 2 commits February 16, 2022 07:40

Merge branch 'main' into writables

d98cf31

Merge branch 'main' into writables

88da8d8

ssbarnea merged commit 2a19a66 into ansible:main Feb 16, 2022

webknjaz reviewed Feb 16, 2022

View reviewed changes

cognifloyd mentioned this pull request Feb 16, 2022

Use pathlib methods instead of open() in Lintable #1913

Merged

This was referenced Feb 17, 2022

Add .idea to gitignore #1915

Merged

Removed unused VALID_KEYS #1918

Merged

cognifloyd mentioned this pull request Feb 26, 2022

Add support for reformatting YAML files #1943

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Lintables to be modified #1884

Enable Lintables to be modified #1884

cognifloyd commented Feb 13, 2022 •

edited

webknjaz commented Feb 13, 2022

cognifloyd commented Feb 13, 2022

lilatomic commented Feb 14, 2022

cognifloyd commented Feb 14, 2022

webknjaz commented Feb 14, 2022

cognifloyd commented Feb 15, 2022 •

edited

webknjaz commented Feb 15, 2022 •

edited

cognifloyd commented Feb 15, 2022

ssbarnea commented Feb 15, 2022 •

edited by cognifloyd

webknjaz commented Feb 15, 2022

webknjaz commented Feb 15, 2022

webknjaz Feb 15, 2022

cognifloyd Feb 15, 2022 •

edited

webknjaz Feb 16, 2022

webknjaz Feb 15, 2022

cognifloyd Feb 15, 2022

webknjaz Feb 15, 2022

webknjaz Feb 15, 2022

cognifloyd Feb 15, 2022

webknjaz Feb 15, 2022

webknjaz Feb 15, 2022

cognifloyd Feb 15, 2022

cognifloyd Feb 15, 2022

webknjaz Feb 15, 2022

ssbarnea commented Feb 15, 2022

cidrblock left a comment

cognifloyd commented Feb 15, 2022

ssbarnea commented Feb 16, 2022

webknjaz left a comment

webknjaz Feb 15, 2022

cognifloyd Feb 16, 2022

cognifloyd Feb 16, 2022

webknjaz Feb 15, 2022

webknjaz Feb 16, 2022

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

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

		with open(self.path.resolve(), mode="w", encoding="utf-8") as file:
		file.write(self._content or "")

	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 "")

Enable Lintables to be modified #1884

Enable Lintables to be modified #1884

Conversation

cognifloyd commented Feb 13, 2022 • edited

webknjaz commented Feb 13, 2022

cognifloyd commented Feb 13, 2022

lilatomic commented Feb 14, 2022

cognifloyd commented Feb 14, 2022

webknjaz commented Feb 14, 2022

cognifloyd commented Feb 15, 2022 • edited

webknjaz commented Feb 15, 2022 • edited

cognifloyd commented Feb 15, 2022

ssbarnea commented Feb 15, 2022 • edited by cognifloyd

webknjaz commented Feb 15, 2022

webknjaz commented Feb 15, 2022

Choose a reason for hiding this comment

cognifloyd Feb 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssbarnea commented Feb 15, 2022

cidrblock left a comment

Choose a reason for hiding this comment

cognifloyd commented Feb 15, 2022

ssbarnea commented Feb 16, 2022

webknjaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cognifloyd commented Feb 13, 2022 •

edited

cognifloyd commented Feb 15, 2022 •

edited

webknjaz commented Feb 15, 2022 •

edited

ssbarnea commented Feb 15, 2022 •

edited by cognifloyd

cognifloyd Feb 15, 2022 •

edited