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

Avoid creating IsFileOfInterest keys for non workspace files #1661

Merged
merged 8 commits into from Apr 6, 2021

Conversation

pepeiborra
Copy link
Collaborator

There's no need to create IsFileOfInterest keys for every interface file

Copy link
Collaborator

@ndmitchell ndmitchell left a comment

Choose a reason for hiding this comment

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

In VS Code you can open individual files that aren't in the workspace. If you have goto definition, then often that is super easy. What happens then?

@pepeiborra
Copy link
Collaborator Author

In VS Code you can open individual files that aren't in the workspace. If you have goto definition, then often that is super easy. What happens then?

  • The file is not in the workspace, so it doesn't get an IsFileOfInterest dependency
  • The file will be VFS, so it gets an alwaysRerun dependency

Now the GetModificationTime for this file has an alwaysRerun dependency forever, i.e. it will never transition to the non alwaysRerun state. This is exactly what we want for non-workspace files, since they are not watched. So all is good.

Copy link
Collaborator

@ndmitchell ndmitchell left a comment

Choose a reason for hiding this comment

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

So it sounds like your argument is if we hit the alwaysRerun path than isFileOfInterest is not interesting. If so, why both with the isWorkspaceFile check? Why not move it down to the branches where alwaysRerun isn't hit, and save ourself the dependency in a more?

I note there are now two local definitions for isWF being whether the file is a workspace file, or a watched file. Would be good to use distinct names.

@pepeiborra
Copy link
Collaborator Author

So it sounds like your argument is if we hit the alwaysRerun path than isFileOfInterest is not interesting. If so, why both with the isWorkspaceFile check? Why not move it down to the branches where alwaysRerun isn't hit, and save ourself the dependency in a more?

Right, that's a good point. I'll move the IsFileOfInterest dependency down to the branch and remove the isWorkspaceFile check.

Copy link
Collaborator

@ndmitchell ndmitchell left a comment

Choose a reason for hiding this comment

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

Thanks - this code is now beautifully understandable - I learnt something about the old code from your rewrite :)

@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Apr 5, 2021
@mergify mergify bot merged commit f31df52 into master Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants