Fix initial position parsing from -pos tag (#4247) #19409
Conversation
This update modifies how the -pos tag is parsed when setting the initial window position. Specifically: - A single value like "number" now sets both x and y positions to that value — similar to how padding behaves. - A value like "number," (with a trailing comma) continues to set only x, leaving y unchanged. Changes: - Updated parsing logic for -pos tag - Added unit test to verify single-value behavior - Confirmed all existing tests pass
microsoft-github-policy-service
bot
added
Issue-Task
|
@microsoft-github-policy-service agree |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thank you so much @carlos-zamora ! This was my first PR ever and I am beyond thrilled to have it approved. This really made my day. Looking forward to contributing and learning more. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Is there any documentation on how to run the full CI test suite locally before pushing? I ran the tests as instructed in the project README, which passed on my system, but the tests here failed. |
|
Yeah! So the problem here is that you're working with an
Yeah! So, the easiest way to run the tests is to do this in PowerShell: # in project root directory
Import-Module .\tools\OpenConsole.psm1
# Use "-Test" to choose a specific DLL to load tests
# (Optional) Add something like "-TaefArgs /name:*settings*" to run all tests with "settings" in the name (you still need -Test to pick the right dll though)
# Alternatively, just use "-AllTests" to run all of the tests, though this takes much longer
Invoke-OpenConsoleTests -Test localTerminalApp -TaefArgs /name:*settings*That said, the local tests don't actually run in CI due to a limitation with TAEF. We're supposed to run them on our own, but admittedly, we haven't done a very good job of that and some of the tests are failing 😅. Instead, I suggest adding tests to something in |
| if (initialPosition.X && !initialPosition.Y && string.find(',') == std::string::npos) | ||
| { | ||
| initialPosition.Y = initialPosition.X; | ||
| } |
There was a problem hiding this comment.
Why don't we modify ParseCommaSeparatedPair to do this for us?
There was a problem hiding this comment.
I felt that function was more general purpose and could be reused elsewhere whereas the LaunchPositionFromString function is specifically for the initialPosition argument. Shall I modify ParseCommaSeparatedPair instead?
3 participants
Summary of the Pull Request
Fixes #4247. Improves consistency in -pos tag parsing by supporting single-value input for both axes.
References and Relevant Issues
#4247
Detailed Description of the Pull Request / Additional comments
The change aligns -pos behavior with padding-style logic. Existing comma-separated behavior remains unchanged.
Validation Steps Performed
PR Checklist
Update --pos tag behavior for single-value input MicrosoftDocs/terminal#890