Skip to content
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

Fix Powershell Formatter appending new line #2239

Merged
merged 10 commits into from
Jan 10, 2023

Conversation

bdovaz
Copy link
Collaborator

@bdovaz bdovaz commented Jan 9, 2023

Right now every time the formatter is run, it adds a new line break.

This PR fixes it:

https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.utility/out-file?view=powershell-5.1#-nonewline

I have the doubt @nvuillam @Kurt-von-Laven @echoix of the file encoding....

https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.utility/out-file?view=powershell-5.1#-encoding

The Out-File command does not preserve the encoding and you have to pass one... Right now I'm passing it in this PR utf8 but I don't know if it's something that should be at least configurable....

Edit: I have not put "default" to the value of encoding because in more recent versions, it does not exist: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.utility/out-file?view=powershell-7.3#-encoding

Which in fact, now I don't know what version of powershell is used in Megalinter, so that's the first thing to find out.

@bdovaz bdovaz requested a review from nvuillam as a code owner January 9, 2023 09:32
@nvuillam
Copy link
Member

nvuillam commented Jan 9, 2023

Isn't it a good practice to finish a code file with a new line ?

@bdovaz
Copy link
Collaborator Author

bdovaz commented Jan 9, 2023

Isn't it a good practice to finish a code file with a new line ?

@nvuillam You didn't understand me, if I don't do this PR, EVERY TIME the linter is executed, it adds a new line, that is, if I execute it 3 times, it adds 3 lines at the end. Look at the documentation I posted for -NoNewline.

@echoix
Copy link
Collaborator

echoix commented Jan 9, 2023

@nvuillam You didn't understand me, if I don't do this PR, EVERY TIME the linter is executed, it adds a new line, that is, if I execute it 3 times, it adds 3 lines at the end. Look at the documentation I posted for -NoNewline.

Oh that's awful

@codecov-commenter
Copy link

Codecov Report

Merging #2239 (5178f51) into main (841b135) will increase coverage by 0.51%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2239      +/-   ##
==========================================
+ Coverage   82.60%   83.11%   +0.51%     
==========================================
  Files         170      170              
  Lines        4484     4484              
==========================================
+ Hits         3704     3727      +23     
+ Misses        780      757      -23     
Impacted Files Coverage Δ
megalinter/linters/PowershellLinter.py 96.29% <ø> (ø)
megalinter/config.py 92.30% <0.00%> (ø)
megalinter/tests/test_megalinter/config_test.py 100.00% <0.00%> (ø)
megalinter/reporters/UpdatedSourcesReporter.py 89.74% <0.00%> (+2.56%) ⬆️
...alinter/tests/test_megalinter/helpers/utilstest.py 89.01% <0.00%> (+8.05%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bdovaz
Copy link
Collaborator Author

bdovaz commented Jan 9, 2023

@nvuillam so what do we do? We leave the value of -Encoding configurable, right? If so:

  1. What value do we set by default?
  2. How do I make it configurable? Do you have any examples of any other linter?

Edit: In fact, this PR is more than important because I see that if for whatever reason the command fails, it pipes anyway and replaces the file content with empty content, but with this PR, it does not continue executing the code.

@nvuillam
Copy link
Member

powrshell version in MegaLinter is currently 7.3.1 -> https://megalinter.io/beta/descriptors/powershell_powershell_formatter/

Of there is an awful bug adding a new lint at each formatting, indeed it's better to never add it :)

@nvuillam
Copy link
Member

nvuillam commented Jan 10, 2023

And everybody probably uses UTF8, but you could add a new variable POWERSHELL_OUTPUT_ENCODING with default utf8, that users could override (and you can get the value of this var in PowershellLinter.py )

@bdovaz
Copy link
Collaborator Author

bdovaz commented Jan 10, 2023

@nvuillam done

Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great <3

@nvuillam nvuillam merged commit e587b4d into oxsecurity:main Jan 10, 2023
@bdovaz bdovaz deleted the fix/powershell-formatter branch January 10, 2023 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants