epacke / BigIPReport

Overview of your loadbalancer configuration
https://loadbalancing.se
8 stars 2 forks source link

the great whitespace cleanup #70

Closed timriker closed 6 years ago

timriker commented 6 years ago

Just the whitespace changes to get things started. No code changes.

timriker commented 6 years ago

In the original, most of the whitespace was already tabs. Spaces were the exception. I just went with what was there. Most of the code was 4 character wide tabs, so I stuck with that. I left spaces in the comments section at the top, as converting that to tabs would mess up the formatting for anyone using 8 character wide tabs. The whitespace in each of the html sequences actually ends up in the output html, so converting the remaining spaces into tabs actually makes the output a little bit smaller.

epacke commented 6 years ago

Yeah, it was a bit mixed before. It's just that I've recently "seen the light" when working in another project. So if it's supposed to change I think that space'd be nicer. Considering that the content is compressed the different should be minimal, right?

timriker commented 6 years ago

It might be nice to preprocess $Global:HTML and remove whatever whitespace we can before writing to reportpath.tmp to compress output. Easy enough to comment out that step when debugging and we'd get smaller html files. I did most of my editing on Linux. The .gitattributes change making *.ps1 files "text" was useful. Moving to spaces instead of tabs would be fine with me. If we had the pre-save whitespace filter then the source formatting would not matter. Can we move to tabs for the moment, look though my other changes, and then move to all spaces?

timriker commented 6 years ago

Testing this now before the write:

   # remove whitespace from output
    if($Outputlevel -ne "Verbose"){
            $Global:HTML = $Global:HTML.Split("`n").Trim() -Join "`n"
    }
epacke commented 6 years ago

I really like that idea. Clever. :)

The changes you've made affects more than just bigipreport. There's some tampermonkey stuff being affected too. Before accepting the PR I'll need to test it. Will try to do so this weekend.

timriker commented 6 years ago

There are a few warnings on the TamperMonkey file too. Is there a reason why you name it .js instead of .user.js? The latter makes it easier to install. There are some missing semicolons, and "var n" being defined twice. I didn't push changes to it as I'm not setup to test it. My changes there were just whitespace removal, no syntax changes.

timriker commented 6 years ago

I'm not sure how large configurations people have. Our individual F5s take over 10 minutes to process. Combining three of them took 88 minutes to run. I think $Global:HTML needs to move to a StringBuilder as powershell string concatenation seems to get much less efficient the larger the strings get. Some performance info here: https://social.technet.microsoft.com/Forums/windowsserver/en-US/95156ee8-e24a-4937-b2ac-548130d96bdc/efficient-string-concatenation?forum=winserverpowershell

epacke commented 6 years ago

Approved and tested. Well done Tim!