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
Convert Web Cmdlets test to OTBS Formating #5716
Conversation
cdf5daf
to
582c065
Compare
@{ Test = @{IntendedProtocol = 'Tls, Tls11'; ActualProtocol = 'Tls12'}; Pending = $IsMacOS } | ||
@{ Test = @{IntendedProtocol = 'Tls11, Tls12'; ActualProtocol = 'Tls'}; Pending = $IsMacOS } | ||
@{ Test = @{IntendedProtocol = 'Tls, Tls12'; ActualProtocol = 'Tls11'}; Pending = $IsMacOS } | ||
@{ Test = @{IntendedProtocol = 'Tls, Tls11'; ActualProtocol = 'Tls12'}; Pending = $IsMacOS } |
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.
Original vertical formatting is better.
Can we configure automating formatting to preserve the vertical aligns?
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.
@iSazonov somehow I missed this comment.
I agree that the original alignment is better, but, it was also inconsistent with how it was done in other areas of this file. I haven't found a way in VSCode to keep this kind of white space alignment or to auto-format single line hashtables like this. The only decent alignment option is the one that puts each key/value on a separate line and aligns the =
's. for a single hash table, that improves readability, but for an array of hashes it actually makes it less readable.
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.
Can we request the feature?
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.
Probably... I wouldn't know how to word it myself. I have an idea of how it should look, but not a good enough definition to even pseudo code it.
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.
I believe the sample that we discuss is good for the request to show that we want.
Or perhaps there are directives to disable this unwanted formatting?
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.
If there is a way to disable it, I certainly haven't found it. I make heavy use of this style for complex logic (silly example):
if
(
$Kizumongatari.Episodes.Count -lt $Bakemonogatari. Episodes.Count -and
$Kizumongatari.Episodes.Count -lt $Nekomonogatari. Episodes.Count -and
$Kizumongatari.Episodes.Count -lt $Nisemonogatari. Episodes.Count -and
$Kizumongatari.Episodes.Count -lt $SecondSeason. Episodes.Count -and
$Kizumongatari.Episodes.Count -lt $Tsukimonogatari. Episodes.Count -and
$Kizumongatari.Episodes.Count -lt $Owarimonogatari. Episodes.Count -and
$Kizumongatari.Episodes.Count -lt $Koyomimonogatari.Episodes.Count
)
{
'Kizumonogatari has the lowest episode count.'
}
And when I started formatting with VScode I lost much of it in my projects. I spent a few days toying with the various settings but it seems like I'd have to just not use any kind of automated formatting if I want to keep this code style.
I believe the sample that we discuss is good for the request to show that we want.
It's one thing to show it, it but it's a whole other thing to define it well enough for someone to implement. Going to the VS Code or PowerShell extension repos with a vague "I would like a formatting option that looks like this" wont be very productive, IMO.
We could split the test file later - it is very large. Thoughts? |
@iSazonov I think it would be good to split this between the 2 cmdlets. I really dislike how massive this file is. Would just need to make sure WebListener can work fine in that scenario as currently it was made under the assumption it only runs in this one test file. The shared test functions need to be moved to a module too. |
Based on discussion above I think we should keep our original vertical formatting for hashes. The rest LGTM. |
@iSazonov I disagree. if we switch back to the vertical formatting it is going to continue to be a pain to keep this file properly formatted. I like the original formatting, but, I also like being able to enforce formatting in this file. We can't have it both ways, but being able to keep this file formatted is the better option. |
Over the past year we have many examples of vertical formatting of hashes in Testcases. Sometimes these hashes are big. We specifically asked the authors to do so to get well-read tests. |
IMO, this needlessly complicates the project. The vertical alignment does add readability but, it's not like it is unreadable without it. It is painful to have to format the document and then go back and discard specific chunks. every. single. time. I have 30 commits to this file and those are only the official squash merges. I have, on average, 3-5 commits to this file per merge addressing PR feedback. Not to mention all the work I do that I end up not submitting PRs for. That is a ton of wasted time dealing with formatting issues in one file or having to just not auto-format it and have to do more commits when the fomating comes up in PR review. It's a waste of your time and my time for so little gain. And that's just me. Asking future contributors to format this file, but be selective raises the bar on who can contribute. If we can simply tell contributors "run 'format this document' in VS Code" and not worry about vertical alignment or unstaging/discarding chunks, we can make this easy and more approachable for new contributors. |
😄 Last February I offered to clean the test files to save the time of others (like you), but the team asked me to spend time on more useful things. I think this is due to a lot of MSFT expert experience - formatting does not make any useful features but can add subtle errors. So we formatting only when it's inevitable or/and for new code. I think this is a very important experience and we have to stick with it. I only considered your formatting because you actually remade the tests. I also hoped that we could use the automatic formatting, but it was not justified. |
Auto-formatting is justified for this file. I am very intimate with it. You could call me an expert on web cmdlets pester tests at this point. I will be contributing to and reviewing changes to this file and the web cmdlets probably for years to come. I'm trying to do everyone (reviewers and contributors alike) a favor here by making auto-formatting possible for this file. I will work on a issue requesting this kind of formatting in the VS Code PowerShell extension. At the very least, I'll ask them to add an "ignore white space in single-line HashTables" option. That way we can still format it by hand and not have the auto-formatting remove it. If that ever gets added we can update the VSCode settings and I will personally format ever hashtable array with vertical alignment in this project. until then, can we please just stick to the auto-formatting??? |
As for your question, I would prefer to have the same rules for all files, otherwise it would mislead other contributors. |
I'm fine to use
I totally understand @iSazonov's concern about the vertical alignment on single-line hashtables as sometimes it's important for readability, such as the For this particular file, the Hashtable arrays for |
@@ -1706,14 +1620,13 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" { | |||
|
|||
# Validate all available user agents for Invoke-RestMethod | |||
$agents = @{InternetExplorer = "MSIE 9.0" |
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.
Maybe you want to move this hash table entry to a new line, like the other $agents
hashtable at line# 616.
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 catch. Thanks! Fixed.
What about other .ps1/.psm1 files? Shall we approve auto format them? Should we mark files which should be auto formatted/non-formatted? |
I don't expect PRs that auto formats a large number of files as that will be impossible to review and possibly not a good use of time. But if it's a particular file like the |
PR Summary
Reference #5669
PR Checklist
Note: Please mark anything not applicable to this PR
NA
.[feature]
if the change is significant or affectes feature testsWIP:
to the beginning of the title and remove the prefix when the PR is ready.