Replace custom method with File.ReadAllText() in ScriptAnalysis.cs#26060
Conversation
…ine\Modules\ScriptAnalysis.cs The ReadScript method was retrieving a SafeFileHandle from the FileStream but never using it. This line was unnecessary since proper disposal is already handled by the using blocks. Changes Removed redundant SafeFileHandle assignment.
|
/azp run PowerShell-Windows-Packaging-CI, PowerShell-CI-linux-packaging |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
This may be a change in behavior, e.g. if the |
|
Yes, removing the SafeFileHandle line does technically change behavior, because the getter could throw in some scenarios (e.g., if the stream was already disposed or in restricted environments). However, since the handle value was never used, this call didn’t contribute to the method’s functionality. The method now only depends on FileStream and StreamReader, both of which are already safely wrapped in using. |
|
The same functionality could be implemented with File.ReadAllText Method. |
|
That’s true ,File.ReadAllText could achieve the same result in a single call. In this PR, my intent was only to remove the unused SafeFileHandle line to clean up the existing implementation without altering its overall structure, as stated in issue #25136 (Extra code in ScriptAnalysis.cs). If replacing ReadScript with File.ReadAllText is desirable, I’d be happy to either update this PR to use File.ReadAllText, or open a follow-up PR focused on that refactoring. |
|
We might as well remove the method and replace the call sites with |
|
I will update this PR with Refactor commit |
The ReadScript helper method has been removed, and all call sites have been updated to use File.ReadAllText(path, Encoding.Default) directly. Changes Deleted the ReadScript method from ScriptAnalysis.cs. Replaced all usages of ReadScript with: File.ReadAllText(path, Encoding.Default)
|
@microsoft-github-policy-service agree |
|
@amritanand-py Please update the PR description. |
|
@xtqqczze Done |
| return result; | ||
| } | ||
|
|
||
| internal static string ReadScript(string path) |
There was a problem hiding this comment.
@MartinGC94 Do you think it is good to remove the method? Won't this code be less readable?
There was a problem hiding this comment.
I think it's perfectly fine. If anything it's arguably easier to understand when it's just File.ReadAllText because you don't have to check if ReadScript does anything weird.
|
/azp run PowerShell-Windows-Packaging-CI, PowerShell-CI-linux-packaging |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
/azp run PowerShell-Windows-Packaging-CI, PowerShell-CI-linux-packaging |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
@amritanand-py Please add dummy commit to unlock merging. |
You can add an empty commit using: git commit --allow-empty --message 'Empty commit' |
|
@xtqqczze Done |
|
@iSazonov It looks like auto-merge isn’t working? |
|
/azp run PowerShell-Windows-Packaging-CI, PowerShell-CI-linux-packaging |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
📣 Hey @@amritanand-py, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
|
@amritanand-py Thanks for your contribution! |
|
Thank you! Glad I could help. Looking forward to contributing more. |
Fix #25136
The ReadScript helper method in src/System.Management.Automation/engine/Modules/ScriptAnalysis.cs was redundant. It created a FileStream and StreamReader only to return the file’s contents, which can be achieved directly with File.ReadAllText(path, Encoding.Default).
This PR removes the ReadScript method and replaces all call sites with File.ReadAllText.
Changes
Removed the ReadScript method.
Updated all usages to:
File.ReadAllText(path, Encoding.Default)PR Summary
This PR simplifies file reading in ScriptAnalysis.cs by removing the unnecessary ReadScript helper method. The built-in File.ReadAllText method provides the same functionality in a cleaner and more efficient way.
Removed:
ReadScript method and replaces all call sites with File.ReadAllText.
PR Context
Extra code in ScriptAnalysis.cs #25136
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header