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

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

Closed
wants to merge 4 commits into from

Conversation

cognifloyd
Copy link
Contributor

@cognifloyd cognifloyd commented Feb 11, 2022

As part of introducing formatting and transforms (see #1828), we need a way to detect changes in lintables.

By introducing a Lintable.open(), we can ensure that

  1. we can detect when changes occur, and
  2. we can reset the internal content cache on change.

The goal of this PR is to achieve (1). (2) is a happy bonus that comes from tracking when the file gets modified.
So, I'm open to any alternative implementation that allows me to detect that the contents change.

Relevant scenarios to consider:

  • File is updated to replace " with '.
  • A file is round-tripped (a read/write cycle) without changing its contents. This should not trigger change detection.

@cognifloyd cognifloyd requested a review from a team as a code owner February 11, 2022 00:51
@cognifloyd cognifloyd requested review from ganeshrn, cidrblock and priyamsahoo and removed request for a team February 11, 2022 00:51
@cognifloyd cognifloyd self-assigned this Feb 11, 2022
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

If I understood correctly the goal was to enable Lintable to detect when the file on disk was modified since it cached its content.

If I would have wrote that, I would have probably look for checking the timestamp of the file and avoid checksum but maybe I am missing something. IMHO, i would have ensured that we would tell a Lintable when to reset cache manually.

For example for the generic formatter I would have looped on all Lintables and tell them to recache if needed, after the initial formatting stage. For Rules that can fix things, I would have just tell the affected Lintable to reload after saving the file.

@webknjaz WDYT?

@cognifloyd
Copy link
Contributor Author

The goal is to report which files were modified.
If I read/write a file with the same contents, then the timestamp will get bumped leading to false positives. So I can't rely on timestamp.

Resetting the cache is a happy bonus. I really only care about recording (somewhere) that a file's contents were modified on disk.

@cognifloyd
Copy link
Contributor Author

I also considered using filesize, but that would not catch updated quotes. If a set of double quotes gets replaced with single quotes, the file size will be the same, but I need to report that the contents have been modified.

@cognifloyd
Copy link
Contributor Author

I created an alternate implementation in #1884 that does not use file content hashing.
I realized that I needed to track more than just an updated bool. To handle some of the UX requests, formatters will need to be able to generate a diff of some kind, so we need both old and new lintable content. #1884 makes those UX options possible and feels much better than this one. Plus, I added a bunch of tests to #1884 so that the purpose is (hopefully) more clear.

Closing in favor of #1884.

@cognifloyd cognifloyd closed this Feb 13, 2022
@ssbarnea
Copy link
Member

We do not need to generate any diffs ourselves, there is git for that and we already rely on it for other features, like finding project root or progressive mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants