Detect deleted files as stale violations #332
Conversation
Breaking it up into private methods for easier readability.
Also rename todo_list to old_entries (to contrast with new_entries), drop Type postfix from TODO entry types, and remove extra file entry from test/fixtures/package_todo.yml to make an existing stale check test pass.
|
A quick benchmark: This patch: Main: So this may add a slight performance regression when calculating deleted files (at a large scale). I don't see a clear way around mapping old entries without changing the serialization format, but we don't have to map new ones (we can instead keep a list of files added as an array). I'll give this some more thought before merging. |
There was a problem hiding this comment.
I took the new and old entries and mapped their files to see the paths that exist in the old TODO, but not the new TODO and added them to the stale check list of files. After implementing this, stale violation checks mean a lot of things, so I think this is a new feature and not a bug.
What other meanings could it have? It still seems like a stale violation if a file is deleted that once contained violations but I'm probably missing something.
| @@ -37,61 +41,39 @@ def listed?(reference, violation_type:) | |||
| params(reference: Packwerk::Reference, violation_type: String).returns(T::Boolean) | |||
| end | |||
| def add_entries(reference, violation_type) | |||
| package_violations = @new_entries.fetch(reference.constant.package.name, {}) | |||
There was a problem hiding this comment.
Maybe not necessary as part of this PR, but wondering if we might want to convert the hash types used for old and new entries to a proper object at some point. Besides more ergonomic typing, the operations on it could be a bit more domain/object oriented.
There was a problem hiding this comment.
Agree. I looked into this but it seemed too complicated for what I was trying to solve. We still need as hash of hashes, and that's where most of the complexity comes from. We may be able to cache file path state in these objects too to cut down on time to find deleted files.
You are correct, I am just basing what our previous definition of what stale meant (a changed violation type or a fixed violation), but I guess everyone I have chatted with also assumed deleted files should be counted too. I'll change the PR to classify this as a bugfix. 👍 |
What are you trying to accomplish?
Closes #298
Detect files deleted in old TODO entries as stale. Also some refactorings along the way.
What approach did you choose and why?
I took the new and old entries and mapped their files to see the paths that exist in the old TODO, but not the new TODO and added them to the stale check list of files.
What should reviewers focus on?
Does this make sense?
Type of Change
Additional Release Notes
Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.
If no additional notes are necessary, delete this section or leave it unchanged.
Checklist