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
Remove most Maximum* capacity variables #2363
Conversation
MaximumErrorCount is fixed at 256 (we may want to consider an environment variable to make this bigger) MaximumAliasCount, MaximumDriveCount, MaximumFunctionCount, and MaximumVariableCount have been removed.
Hi @lzybkr, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maxErrorCount was previously limited to 32768. I feel we should leave it at that for backwards compatibility.
@PaulHigin - can you clarify your comment? I don't see a compatibility issue. I can't see folks relying on >256 errors in an automation scenario. Maybe in rare cases you need >256 errors retained to investigate a failure, but that's a manual step, so it's not really a compatibility issue. Setting |
I meant that if users depend on having more than 256 errors in the error list then this limits them unnecessarily. But I agree it is not a large issue and probably unlikely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
driveInfos.Count > DriveCapacity.FastValue - 1) | ||
{ | ||
SessionStateOverflowException e = | ||
new SessionStateOverflowException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that SessionStateOverflowException
only gets constructed and thrown from within SessionStateScope.cs
-- there are 7 references to this constructor and all of them are in this file.
We probably have to keep the class SessionStateOverflowException
since it's a public API, even though it's not used anymore. But there are a lot /// <exception cref="SessionStateOverflowException">
in comments, which we probably should clean up somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - I've cleaned this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lzybkr for cleaning it up!
This exception is no longer used by PowerShell, so it has been removed. It is a public api, so removing it risks breaking somebody. Nobody has any reason to catch or throw this exception, so it seems safe to remove, but I decided to be conservative and keep it for Windows PowerShell. It seems safe enough to remove in Nano.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This change broke one of my unit tests, the test was confirming that '-ErrorAction SilentlyContinue' was working in a function. The test was lazy, it just checked that $Error.Count had increased, or had hit the $MaximumErrorCount. Is this change from PowerShell 5 documented anywhere? I've been testing my unit tests against PowerShell v7.0.0-rc.1, and have not spotted anything about breaking changes (admittedly this is very much an edge case). It's not mentioned in C:\Program Files\PowerShell\7-preview\Modules\PSReadLine\Changes.txt. |
@DavidGibsonNorth This change was made more than 3 year ago during the PowerShell v6.0 time frame. It was mentioned in the
That file only contains changes for the |
Ok, thanks for that. Fair enough. I missed out PowerShell 6 due to the narrower scope, version 7 working well, it's only requiring the odd 1 line tweak, found by unit tests. |
I was load testing some PowerShell code that uses a background runspace to ship all the errors that have been written to $Error. I ran into the 256 item limit. I struggled to figure out why the $MaximumErrorCount variable mentioned in several articles wasn't working until I tried going back to PowerShell 5.1. After realizing it was just broken after Powershell 5.1 I adjusted my web search and found this thread. Paul clearly calls out the issue of backward compatibility and avoiding an unnecessary limitation. Instead a decision was made based on a faulty argument that removing functionality isn't an issue of backward compatibility and also on not "seeing folks using it". Regarding this change being made three years ago, I would suggest that people aren't noticing until now because their migration to .Net-Core-Based PowerShell has been delayed potentially due to issues like this. Please reinstate the ability to increase the MaximumErrorCount because people need it, it had already been implemented practically, and there cases when it is preferable to using -ErrorVariable or clearing the $Error variable as it fills up. |
MaximumErrorCount is fixed at 256 (we may want to consider an environment variable to make this bigger)
MaximumAliasCount, MaximumDriveCount, MaximumFunctionCount, and MaximumVariableCount have been removed.
Fixes #2221