Skip to content

Fix issue 11820 Add DirectoryExists() and FileExists() function#11919

Merged
YuliiaKovalova merged 4 commits intodotnet:mainfrom
huulinhnguyen-dev:dev/huulinhnguyen/issue11820
Jun 2, 2025
Merged

Fix issue 11820 Add DirectoryExists() and FileExists() function#11919
YuliiaKovalova merged 4 commits intodotnet:mainfrom
huulinhnguyen-dev:dev/huulinhnguyen/issue11820

Conversation

@huulinhnguyen-dev
Copy link
Copy Markdown
Contributor

@huulinhnguyen-dev huulinhnguyen-dev commented May 28, 2025

Fixes #
Fix the issue 11820, add two function DirectoryExists() and FileExists() #11820

Context

Currently, doesn't have any goodway to check the File or the Directory is exist or not, and the [System.IO.Directory]::Exists() is not allowed.

Changes Made

Add two function DirectoryExists() and FileExists() in the msbuild, now we can use [MSBuild]::FileExist() or [MSBuild]::DirectoryExists() in the project files

Testing

I have performed testing for various exception to validate the changes. Tested two case FileExists and DirectoryExist

Notes

@huulinhnguyen-dev huulinhnguyen-dev marked this pull request as ready for review May 28, 2025 04:22
Copy link
Copy Markdown
Member

@SimaTian SimaTian left a comment

Choose a reason for hiding this comment

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

Nice one. Thank you for your help.

@rainersigwald
Copy link
Copy Markdown
Member

rainersigwald commented May 28, 2025

This looks like a good implementation, but my first instinct is to add [System.IO.Directory]::Exists(string), since that mirrors what we support today for files.

@KirillOsenkov, you asked for [MSBuild]::FileExists and [MSBuild]::DirectoryExists in #11820 (comment) though--you'd prefer this to enabling [System.IO.Directory]::Exists?

@KirillOsenkov
Copy link
Copy Markdown
Member

KirillOsenkov commented May 28, 2025

image

I think we should have FileExists, DirectoryExists and [System.IO.Directory]::Exists too. It's strange that it's currently not available.

Copy link
Copy Markdown
Member

@KirillOsenkov KirillOsenkov left a comment

Choose a reason for hiding this comment

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

👍

@rainersigwald
Copy link
Copy Markdown
Member

Makes sense, I also favor "both/and".

@huulinh99, would you be interested in enabling Directory.Exists as part of this PR? It should be here:

availableStaticMethods.TryAdd("System.IO.Directory::GetDirectories", directoryType);
availableStaticMethods.TryAdd("System.IO.Directory::GetFiles", directoryType);
availableStaticMethods.TryAdd("System.IO.Directory::GetLastAccessTime", directoryType);
availableStaticMethods.TryAdd("System.IO.Directory::GetLastWriteTime", directoryType);
availableStaticMethods.TryAdd("System.IO.Directory::GetParent", directoryType);

@huulinhnguyen-dev
Copy link
Copy Markdown
Contributor Author

Hi @rainersigwald , thank you so much for your comment, I will enable Directory.Exists in this PR

@KirillOsenkov
Copy link
Copy Markdown
Member

Quick question, will the syntax such as Condition="Exists('foo')" be supported? I'd like Condition="FileExists('foo')" and Condition="DirectoryExists('foo')" to work.

@rainersigwald
Copy link
Copy Markdown
Member

I'd like Condition="FileExists('foo')" and Condition="DirectoryExists('foo')" to work.

That is entirely separate (😔) . . . but a reasonable request.

@KirillOsenkov
Copy link
Copy Markdown
Member

KirillOsenkov commented May 29, 2025

wait, there are three syntaxes?

I guess ideally I was asking for the syntax without the ::

😅

@huulinhnguyen-dev
Copy link
Copy Markdown
Contributor Author

Hi @rainersigwald @KirillOsenkov Could you confirm again that you are expecting both syntax Condition="$([System.IO.Directory]::Exists('foo'))" and Condition="DirectoryExists('foo')" to work well?

@KirillOsenkov
Copy link
Copy Markdown
Member

Rainer says it's a separate mechanism, perhaps we can merge this PR and do that syntax separately, or do it as part of this PR, or not do it at all.

My apologies that the scope keeps increasing :)

Let's see what Rainer thinks. When I originally filed this issue, I wanted DirectoryExists() and FileExists() to complement the Exists() syntax, because that's the short and most natural one, and I'd love for this syntax to be used everywhere. I'm not sure how hard would it be to add support.

Copy link
Copy Markdown
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Yeah we don't have to add any more functionality to this PR, let's merge with this (after adding the Directory.Exists test).

I would be happy for a follow-up PR to add the condition "functions" in FunctionCallExpressionNode.cs though!

@huulinhnguyen-dev
Copy link
Copy Markdown
Contributor Author

Thank you for your comment @rainersigwald @KirillOsenkov , I will do base on your suggestion

@huulinhnguyen-dev
Copy link
Copy Markdown
Contributor Author

@dotnet-policy-service agree

Copy link
Copy Markdown
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@YuliiaKovalova YuliiaKovalova merged commit ac0e764 into dotnet:main Jun 2, 2025
10 checks passed
@KirillOsenkov
Copy link
Copy Markdown
Member

Thank you so much @huulinhnguyen-dev!

@GangWang01 GangWang01 linked an issue Jun 12, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add DirectoryExists() function or allow Directory::Exists

5 participants