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

Correct type guard for FileStat.is #8986

Merged
merged 1 commit into from
Jan 27, 2021
Merged

Correct type guard for FileStat.is #8986

merged 1 commit into from
Jan 27, 2021

Conversation

westbury
Copy link
Contributor

What it does

This is a fix to a copy-and-paste error. A new test was created a few months back in Theia 1.5 for FileStat. However the type guard was left at BaseStat, meaning one can't access isDirectory etc. after testing for a FileStat.

How to test

This cannot be tested without making modifications to the code.

Although FileStat.is is used a few times in the Theia codebase, in most cases the code does not access FileStat properties afterwards. In the cases where the FileStat property isDirectory is used, the original type is URI | URIIconReference | FileStat, typescript being smart enough to determine that if the type is BaseStat then it must be FileStat.

Review checklist

Reminder for reviewers

Signed-off-by: Nigel Westbury <nigelipse@miegel.org>
@westbury westbury added the filesystem issues related to the filesystem label Jan 25, 2021
@colin-grant-work
Copy link
Contributor

I confirm that the current code spoils code intelligence about a successful FileStat.is check, and that the new code addresses the problem.

@colin-grant-work colin-grant-work merged commit 34689a5 into master Jan 27, 2021
@github-actions github-actions bot added this to the 1.10.0 milestone Jan 27, 2021
@vince-fugnitto vince-fugnitto deleted the file-stat-type branch January 27, 2021 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filesystem issues related to the filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants