PSScriptAnalyzer icon indicating copy to clipboard operation
PSScriptAnalyzer copied to clipboard

PSAvoidTrailingWhitespace should ignore empty lines indented to match the current block

Open poshcodebear opened this issue 7 years ago • 18 comments

Currently, PSAvoidTrailingWhitespace checks all lines for trailing whitespace, even when that line is effectively empty and the "trailing" whitespace is actually leading whitespace for the current indentation level. If the recommended action is taken (remove the whitespace), then adding new code in those spots requires re-indenting that spot to the current code indentation level.

I propose that it should respect the current indentation level for an empty line and not generate a note for those lines, as long as they match the indentation of the current block of code.

What is the latest version of PSScriptAnalyzer at the point of writing 1.17.1, shipping with the vscode-powershell extension version 1.8.1

poshcodebear avatar Jul 11 '18 21:07 poshcodebear

Thanks for the feedback. Which editor do you use that results in having such lines? I think the default behaviour is maybe ok but having a configuration option for the rule to ignore lines with whitespace only would make sense to me.

bergmeister avatar Jul 11 '18 21:07 bergmeister

As implied, I use Visual Studio Code; that being said, every modern editor I've used (VSCode, ISE, Notepad++, Atom, Visual Studio) does the exact same thing: retains the current indentation line when I press enter. If I hit enter twice, I have an empty line at the same indentation level as the code around it.

I would be fine with that solution, assuming it doesn't mean having to create a settings file for every repo I work in just for that one setting.

poshcodebear avatar Jul 11 '18 21:07 poshcodebear

Ok. I see. In VSCode one can set the setting files.trimTrailingWhitespace to true and on saving the document the trailing whitespace will get removed automatically. One can even tweak the rule to make it only apply to PowerShell files as follows:

 "[powershell]": {
    "files.trimTrailingWhitespace": true
  },

bergmeister avatar Jul 11 '18 21:07 bergmeister

I'm not so much looking for it to automatically handle it for me; additionally, I want the automatically inserted whitespace that matches the indentation level to remain (this makes future changes easier as I don't have to stop and re-indent lines that should already be indented correctly).

Up until the last update to the PowerShell extension which pushed this latest version of PSScriptAnalyzer, I've not had to create a settings file to control its behavior as it's been fine out of the box. I get an update and all of a sudden it's complaining about perfectly legitimate use of whitespace and I'm finding the only way to get it to quiet down is either to manually turn that check off every time I launch or create a settings file for every PowerShell folder I work with. Neither of these solutions are particularly appealing, and I don't want to turn it off either, but I find the behavior that it insists on baffling and without any explanation (the documentation/description only lists issues with trailing whitespace at the end of code lines, not at the end of blank lines).

Anyway, if the behavior won't be changed, I feel that the description should at least be updated with a defense of that as a standard.

poshcodebear avatar Jul 11 '18 21:07 poshcodebear

OK, I see now where you're coming from and understand your pain. My current thinking is that it might be be better to rather change the defaults in the VSCode extension to use a new customisation of this rule to allow whitespace only lines. I think we could discuss the defaults of this customisation afterwards depending on what other people think but having the customisation and using it in the defaults of the extension (to be precise, it is actually PowerShellEditorServices, which applies to other editor as well) sounds reasonable. Since this depends on a new PSSA release, would it be better to turn this rule not on by default for the moment @tylerl0706 ?

bergmeister avatar Jul 11 '18 22:07 bergmeister

every modern editor I've used (VSCode, ISE, Notepad++, Atom, Visual Studio) does the exact same thing: retains the current indentation line when I press enter. If I hit enter twice, I have an empty line at the same indentation level as the code around it.

The indentation is only retained if something has been typed (including if it was subsequently deleted). It shouldn't stick around with a double tap of enter. At least it doesn't for me, maybe that's a setting.

That said, it can be annoying when it reports the indent of the current line though. I've gotten used to that from TSLint (which works the same way), but it would be cool if we could ignore the warning for the current line. That would have to be done on the PowerShellEditorServices side though, because we don't report cursor status to PSSA.

would it be better to turn this rule not on by default for the moment

👍

SeeminglyScience avatar Jul 12 '18 17:07 SeeminglyScience

Ok, I created a PR in the PSES repo to not have the rule enabled by default for the moment as a quick solution

bergmeister avatar Jul 12 '18 21:07 bergmeister

Yes, this was quite a surprise when, all of a sudden, I saw hundreds of alerts in my code. I'm amazed no one picked this up before it was pulled into the main code, as every code editor adds tabs/spaces to indent blank lines to the level of the preceding code as a default setting.

stetan avatar Jul 14 '18 22:07 stetan

@SeeminglyScience filtering out the PSSA warnings that contain the cursor position shouldn't be too hard to add.

I don't think I've seen any other linters do something like that but it makes sense to me.

TylerLeonhardt avatar Jul 15 '18 21:07 TylerLeonhardt

Not something I'd use, but here is the setting from ESLint https://eslint.org/docs/rules/no-trailing-spaces#options

nschonni avatar Jul 16 '18 04:07 nschonni

Moving the remaining issue to the PSES repo.

rjmholt avatar Aug 20 '18 18:08 rjmholt

Just my 2 cents -- this is something I'd use this rule exactly to check; I understand there are differing opinions, but both my habitual editing experience (in VSCode, vim, etc.) and my preferred style is to get rid of extraneous, orphaned whitespace.

As a linter, I feel like PSScriptAnalyzer should support both opinions (that's what a linter is in my mind, a way to formally specify and apply code style opinions).

rjmholt avatar Aug 20 '18 21:08 rjmholt

Going to reopen since @bergmeister has a PR open on this

rjmholt avatar Aug 20 '18 21:08 rjmholt

@rjmholt I see where you're getting at, and agree that it's a personal style choice; I think it's great to have it as an option, it's just one I've not been able to turn off (short of setting up linting rules in all my repos, which I really didn't want to do for this).

The whitespace on a line by itself matching the indentation of the lines around it being flagged, in my opinion, should really be a separate rule from trailing whitespace for just this reason.

The other issue, of course, is that there should be a way to turn rules on and off globally and not have to set up config files manually, but that's a separate issue.

poshcodebear avatar Aug 20 '18 21:08 poshcodebear

This rule was created because there are lots of problems that arise specifically in PowerShell as a result of hidden whitespace. The number one that we found was trailing spaces after backticks which would escape the space rather than the line break, preventing stuff like this from working correctly:

Get-Foo -Bar Hello`<if there's a space here you have a problem>
        -Baz World

And while whitespace-only lines don't cause the same sorts of issues in PowerShell, it does create issues in source control where people's editors are different (and this is specifically in place to help with that scenario).

So yeah, my take is that this rule is working as designed.

As for globally changing your settings configuration, have you tried using this:

// Specifies the path to a PowerShell Script Analyzer settings file. To override the default settings for all projects, enter an absolute path, or enter a path relative to your workspace.
  "powershell.scriptAnalysis.settingsPath": ""

joeyaiello avatar Aug 21 '18 21:08 joeyaiello

This is a couple years old and still an open issue.

Can this rule be updated (or a new rule created) in order to fix the VSCode formatting issue? I am applying the change to VSCode as in above, however whitespace on a blank line (to match indenting between two lines) seems like a proper pattern and it shouldn't break backticking...

By default, VSCode will break this rule which causes added issues.

Or can close this with a won't fix and will move on to trying to change the defaults for VSCode's formatter.

asears avatar Sep 03 '20 11:09 asears

For what it's worth, I'm in the "a line with nothing but whitespace on it is a mistake and a waste of space" camp.

There are many reasons that those should be cleaned up, not least of which is that they are meaningless.

Are there really linters in any other languages that clean trailing whitespace but don't clean it if it might someday be leading whitespace?

Anyway. I have no problem with giving people the option to ignore this, but it seems there's currently no option to clean them up, so I have to use both ScriptAnalyzer and the VS Code setting to clean them up.

Jaykul avatar Nov 17 '20 18:11 Jaykul

Since there are different opinions, there should be different setting for how AvoidTrailingWhitespace behaves.

Besides, if there is nothing at the start of the line (except white spaces) what exactly is the white spaces trailing?

DennisL68 avatar Jun 15 '23 17:06 DennisL68