Skip to content

Remove Utils.FileExists(), Utils.DirectoryExists() and Utils.ItemExists() methods #7100

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.

Sign up for GitHub

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

Jump to bottom
Closed
iSazonov wants to merge 27 commits into from
Closed

Remove Utils.FileExists(), Utils.DirectoryExists() and Utils.ItemExists() methods #7100

iSazonov wants to merge 27 commits into from

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Jun 18, 2018

PR Summary

Continue #6929

Cleanup code to replace Utils.FileExists(), Utils.DirectoryExists() and Utils.ItemExists() methods with .Net Core methods.

PR Checklist

adityapatwardhan
adityapatwardhan requested changes Jun 18, 2018
View reviewed changes
src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated
Copy link
Member

@adityapatwardhan adityapatwardhan Jun 18, 2018

Choose a reason for hiding this comment

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

return was within the catch block.

Copy link
Collaborator Author

@iSazonov iSazonov Jun 19, 2018

Choose a reason for hiding this comment

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

@adityapatwardhan Could you please clarify why we should move the return in catch block to continue recursion removal?
Previous code was trying the recursion removal if only we can not remove reparse point silently without any error and exception - is it real scenario?

Copy link
Member

@adityapatwardhan adityapatwardhan Jun 20, 2018

Choose a reason for hiding this comment

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

@iSazonov misread the diff. Signed off.

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Jun 20, 2018

@iSazonov Please have a look at the test failures.

@iSazonov iSazonov force-pushed the cleanup-file-exists branch from 4b63525 to a0acbd0 Compare June 20, 2018 05:56
@iSazonov
Copy link
Collaborator Author

iSazonov commented Jun 20, 2018

Rebased to get latest fixes.

@iSazonov iSazonov force-pushed the cleanup-file-exists branch from a0acbd0 to 68d9f25 Compare June 20, 2018 08:05
@iSazonov iSazonov force-pushed the cleanup-file-exists branch from 68d9f25 to 2d28133 Compare June 20, 2018 10:10
@iSazonov
Refactor FileSystemProvider.NormalizeThePath() method
1c1386b 
Exclude GetFileSystemInfo() and Utils.FileExists()
@iSazonov iSazonov force-pushed the cleanup-file-exists branch 3 times, most recently from e629b77 to 132ef5e Compare June 20, 2018 14:58
iSazonov added 14 commits June 20, 2018 20:15
@iSazonov
Use Directory.Exists in FileSystemProvider.cs
10f481e
@iSazonov
Remove Utils.DirectoryExists()
4e5bd0d
@iSazonov
Exclude Utils.ItemExists() from FileSystemProvider.ItemExists()
bb22d3c
@iSazonov
Exclude Utils.ItemExists() from FileSystemProvider.NewItem()
a99bd63
@iSazonov
Exclude Utils.ItemExists() from FileSystemProvider.RemoveDirectoryInf…
abff189 
…oItem()
@iSazonov
Exclude Utils.ItemExists() from FileSystemProvider.GetProperty()
666de17
@iSazonov
Exclude Utils.ItemExists() from FileSystemProvider.NewItem() - 2
f9f08c7
@iSazonov
Exclude Utils.ItemExists() from FileSystemProvider.NewItem() - 3
85a9865
@iSazonov
Exclude Utils.ItemExists() from FileSystemProvider.NewItem() - 4
b81b3c5
@iSazonov
Exclude Utils.ItemExists() from FileSystemProvider.GetPathItems()
f678fe9
@iSazonov
Exclude Utils.ItemExists() from FileSystemProvider.SetProperty()
f57ec8a
@iSazonov
Exclude Utils.ItemExists() from FileSystemProvider.NormalizeRelativeP…
08b2e99 
…ath()
@iSazonov
Remove Utils.ItemExists()
4dc4838
@iSazonov
[Feature] Exclude Utils.ItemExists() from security\Utils.cs
0172040
@iSazonov iSazonov force-pushed the cleanup-file-exists branch from 132ef5e to 0172040 Compare June 20, 2018 15:28
@iSazonov
Copy link
Collaborator Author

iSazonov commented Jun 20, 2018

I update some commits:

  • Fix typo if ((int)item.Attributes != 1) -> if ((int)item.Attributes != -1)
  • Replace wrong pattern isDirectory = attrs.HasFlag(FileAttributes.Directory); with isDirectory = exists && attrs.HasFlag(FileAttributes.Directory);
  • Fix CodeFactor issues.

Seems I fixed all Appveyor CI issues. I can not build Unix version locally so I haven't a progress with Travis CI issues.

daxian-dbw
daxian-dbw reviewed Jun 20, 2018
View reviewed changes
src/System.Management.Automation/namespaces/FileSystemProvider.cs
FileSystemInfo fsinfo = new FileInfo(path);
var attr = fsinfo.Attributes;
var exists = (int)attr != -1;
isContainer = exists && attr.HasFlag(FileAttributes.Directory);
Copy link
Member

@daxian-dbw daxian-dbw Jun 20, 2018

Choose a reason for hiding this comment

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

I see code like this for checking if a path exists and whether it's directory at 8 places. I think keeping the redundant code in Utils.ItemExists is better. Thoughts?

Copy link
Collaborator Author

@iSazonov iSazonov Jun 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I agree. I was hoping to remove "unnecessary" checks of existence at all but it's too complicated the PR. I suppose the correct approach is that an operation is performed and an exception is handled instead of up to 6 times to check for each file's existence.
I think I'll close the PR and open new one where add commit by commit to discover and fix Unix issues and replace ItemExists with CheckItemExistsAndThrowOnError .

Copy link
Member

@daxian-dbw daxian-dbw Jun 21, 2018

Choose a reason for hiding this comment

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

Thanks @iSazonov

@iSazonov iSazonov mentioned this pull request Jun 21, 2018
Merged
11 tasks
@iSazonov
Copy link
Collaborator Author

iSazonov commented Jun 21, 2018

Continue in #7129

@iSazonov iSazonov closed this Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw left review comments

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@anmenaga anmenaga Awaiting requested review from anmenaga

@BrucePay BrucePay Awaiting requested review from BrucePay

@Francisco-Gamino Francisco-Gamino Awaiting requested review from Francisco-Gamino

Assignees

@daxian-dbw daxian-dbw

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iSazonov @adityapatwardhan @daxian-dbw