Feature - Rejected <shout>SPRING CLEANING!!</shout> -- whitespaces

fredg1

Member
It's Spring! And it means two things:
1- Taco Season (one day, I'm telling you...)
2- Spring cleaning!


So... uh... I kind of snapped, and went/am going on a cleaning rampage throughout mafia...
For now, let's start "simple": Whitespaces. Spaces, tabs, and newlines, to be exact.
A hyyuuuuuuge buttload of nothing but whitespace changes.

I tried very hard to have all of mafia conform to the intended style guidelines.


This was most likely not ALL of them, but I'm very confident that I managed to find the vast majority. (Except a few files like ChoiceManager, as I'm currently in the middle (actually more like the end) of a big rework of them, and changing them would give no benefit other than throwing in some merge conflicts on my side).

There was no clear indications, so I had to "take a gamble" on some.
The one I'm the least sure about is the array declaration; I wasn't sure whether we wanted
Java:
String[] name
or
Java:
String [] name
I went for the former, hope I wasn't wrong ._.

This was all done BY HAND. If there was any mistake, let me know; there's an actual human on the other side of the screen who will learn from this.

(literally had to make 2 patch files because I was busting the site's upload limit...)

Edit: removed group 1 and 2 mentioned in https://kolmafia.us/threads/shout-spring-cleaning-shout-whitespaces.25975/post-161844
 

Attachments

  • WHITESPAAAACEEES1_1.patch
    862.8 KB · Views: 1
  • WHITESPAAAACEEES2_1.patch
    706 KB · Views: 0
Last edited:

fronobulax

Developer
Staff member
I won't ask "why" and I won't bore you with the consequences I experienced when "cleaning code" and inadvertently breaking something :)
 
Last edited:

philmasterplus

Active member
Does Java have a "code formatter" like Prettier? It would be better to run an automated tool run against the codebase. We could even set up something that runs on svn push.
 

MCroft

Developer
Staff member
a few days ago, I pointed Fredg1 at the IJ formatting file that I got from Veracity. I'm pretty sure it was the starting point.

I like standards, but I don't have preferences.
 

fronobulax

Developer
Staff member
I like standards and have preferences. I have been asked to revert commits in order to adhere to a standard and not my preference, although it has been several years since that happened. There was also a period when not all of the devs had set things up so checked in code had a consistent EOL. Because I have broken things when I thought my changes were purely cosmetic, I tend to make this kind of change on a file by file basis and usually only when I am preparing to commit the file. Even then I got growled at because someone using a diff tool chose not to have their tool ignore whitespace and so my commit triggered their alarm bells.
 

fredg1

Member
This patch is nothing but trailing spaces/tabs, double spaces (when there was clearly only meant to be one), tabs used as spaces, spaces used for/in indentation (EXCEPT when used to align statements), spaces between parenthesis/brackets/braces (EXCEPT when the parenthesis are used for type conversion, OR bit-wise operations)

Here are the only exceptions (I went through the whole thing again just to check the list was exhaustive):

Group 1:
persistence.SkillDatabase, line 778/779
swingui.menu.LootHunterMenuItem, line 182
request.FightRequest, line 6245/6249
session.DvorakManager, line 90
swingui.GenericFrame, line 305/564/686/693/700
swingui.MaximizerFrame, line 353/378
swingui.StoreManageFrame, line 465
webui.BarrelDecorator, line 244
webui.BarrelDecorator, line 334
CoinmasterData, line 352
FamiliarData, line 297/299
MonsterData, line 753/824/895/907

I cleaned something nearby or was about to clean a parenthesis (only one of the two had a space or something), when I noticed that the parenthesis did nothing, so I removed it --
I already regret these. I said that it was a whitespace only patch, and this is what it's going to be, so I'm going to make the patch again with those removed (I mean... re-added?)

Group 2:
request.FloristRequest, lines 444-461

removed the braces inside the "case" --
Same as Group 1... Sorry, I don't know what got into me, I'll undo those right away...

Group 3:
swingui.menu.PartialMRUList, line 99; a comment clearly meant as a JavaDoc was missing an asterisk (/* instead of /**) to be made into one.

Group 4:
request.FightRequest, line 8563
Code:
             !image.equals(" lolmecidol.gif" ) )
was clearly meant to be
Code:
             !image.equals( "lolmecidol.gif" ) )
. It was never caught because the spelunky gold is parsed right away through charpane.php anyway.

Group 5:
textui.renderer.AnsiSerializer &&
utilities.ColorParser &&
KoLmafiaTUI

added the KoLmafia license at the start, as it was missing




That is IT. Everything else is nothing but whitespace changes.
 

fredg1

Member
Here. This is the patch with group 1 and 2 removed/undone.

I'm ready to do anything to come to an agreement; all I want is consistency (and for that consistency to be achieved under 10 years).

Is there something I did wrong? A change I shouldn't have done? Just say it, I'll go through this all over again to undo/correct it, seriously. I just can't stand always being on the lookout for those, knowing there could be one anywhere...
 

Attachments

  • WHITESPAAAACEEES1_1.patch
    862.8 KB · Views: 0
  • WHITESPAAAACEEES2_1.patch
    706 KB · Views: 0

fronobulax

Developer
Staff member
tabs used as spaces

I know there is a KoLmafia standard about this which I cannot find or remember with certainty, but in 2011 I said

My IDE is set to not expand tabs to spaces but I don't really trust the setting.

which suggests the standard is to use (leading) tabs and assume they expand to 4 spaces. @MCroft will come along and remind me exactly what the standard is because he made an IntelliJ settings file that let the IDE comply with the standard.

In my effort to infer standard from my settings I noticed that several of the things you have done can be enforced by IntelliJ and presumably any properly configured it. In the same thread cited above veracity's response to IDEs that changed formatting was "Don't do that". Perhaps a decade later there is a different answer :)
 

fredg1

Member
I know there is a KoLmafia standard about this which I cannot find or remember with certainty, but in 2011 I said

"My IDE is set to not expand tabs to spaces but I don't really trust the setting. "

which suggests the standard is to use (leading) tabs and assume they expand to 4 spaces. @MCroft will come along and remind me exactly what the standard is because he made an IntelliJ settings file that let the IDE comply with the standard.

In my effort to infer standard from my settings I noticed that several of the things you have done can be enforced by IntelliJ and presumably any properly configured it. In the same thread cited above veracity's response to IDEs that changed formatting was "Don't do that". Perhaps a decade later there is a different answer :)
The reason you're not letting it turn spaces to tabs is most likely because they are also used for "alignment"

example:
Java:
            if ( true &&
                 false ||
                 false ||
                 true ||
                 false &&
                 true )
\t  \t  \t  \s\s\s\s...

IDEs most often won't understand it and will turn the group of 4 (or whatever you set your tab length at) spaces into a tabulation, un-aligning the arguments for anyone using a different tab length

I did NOT touch those. Like I said, I even fixed a lot of such misalignments
 

fronobulax

Developer
Staff member
Here. This is the patch with group 1 and 2 removed/undone.

I'm ready to do anything to come to an agreement; all I want is consistency (and for that consistency to be achieved under 10 years).

Is there something I did wrong? A change I shouldn't have done? Just say it, I'll go through this all over again to undo/correct it, seriously. I just can't stand always being on the lookout for those, knowing there could be one anywhere...

If it ain't broke than don't fix it. This is something that is only broke for one person, you. If anyone else noticed or cared you would have nothing to correct :) In my career I did not obsess about formatting but I loved to run lint to "clean up" code. Consequences of that were a) I discovered bugs in lint that broke post-lint code in subtle ways; b) I got pinged for checking in code that no longer met what at the time was an unwritten standard (although the standard get documented after I violated it); c) I got dinged for checking in code that broke something else (because in Ye Olden Days developers did not have access to test tools and suites); and d) I got a stern talking to because I wasn't actually supposed to charging time to the project when I ran lint.

KoLmafia is not a professional project so none of these things are likely to be relevant. But they do mean I approach changes like this with caution. Consequently I personally am not comfortable committing this patch. If someone else is comfortable I won't object but I will gleefully say "I told you so" if something turns out to have been broken.

I think you observed this but please don't change anything, even whitespace, in the code under lib. That has evolved to contain code where we could not get a jar file and preserving the originator's formatting makes it easier to copy the code from the originator, perhaps reintegrating changes that KoLmafia needed to make.
 

fredg1

Member
In my career I did not obsess about formatting but I loved to run lint to "clean up" code. Consequences of that were a) I discovered bugs in lint that broke post-lint code in subtle ways; b) I got pinged for checking in code that no longer met what at the time was an unwritten standard (although the standard get documented after I violated it); c) I got dinged for checking in code that broke something else (because in Ye Olden Days developers did not have access to test tools and suites); and d) I got a stern talking to because I wasn't actually supposed to charging time to the project when I ran lint.
You say this... as if I also had used a tool of some sort to make this? Again, this was all done by hand, so none of these apply. There's no "removing unused imports", no "turning
Code:
for ( int i = 0; i < <array>.length; i++ )
loops into
Code:
for ( <member> : <array> )
, no changing of variable/function name, etc, etc, etc.
This. Patch. Is. Only. Whitespaces.

There are other things I want to do, like adding generics to a lot of raw types (and even then, those have no impact at run-time whatsoever), but I didn't include them, specifically to make it so that there would be no reason to not accept this patch other than "one of the things I did was not the style we want".
 

lostcalpolydude

Developer
Staff member
Fronobulax has come along with plenty of statements about how bad things could potentially happen when simply fixing formatting.

From my perspective, it's more a case of "who cares?" I fix formatting stuff locally if I ever notice it, but I never check in those changes, until there's an actual code change to the file months or years later. (I also don't actually apply people's patches when I look at them; instead I open them up and copy-paste everything, to make sure I actually see everything being changed. My workflow probably isn't the best, but it would certainly be a nightmare when looking at a whitespace-only change.)

Your frustration doesn't really change the motivation to apply the patch. I assume your motivation is that you have other patches you want to submit, and you don't want to have whitespace changes detracting from the actual purpose of those patches?
 

gausie

D̰͕̝͚̤̥̙̐̇̑͗̒e͍͔͎͈͔ͥ̉̔̅́̈l̠̪̜͓̲ͧ̍̈́͛v̻̾ͤe͗̃ͥ̐̊ͬp̔͒ͪ
Staff member
@fredg1 as I've said to you many many times you would strongly benefit from:
  1. discussing your intended changes with others in the community before putting a lot of work into them.
  2. acting at all on the feedback you get when you discuss your changes
I think that you are smart and dedicated and I appreciate many of the changes you work on. I really hate to see you spend time on something only for it to be met with hesitancy, skepticism or outright refusal - it pains me! But my solution to that is only ever going to be to reiterate the advice I've given above.
 

fronobulax

Developer
Staff member
You say this... as if I also had used a tool of some sort to make this? Again, this was all done by hand, so none of these apply. There's no "removing unused imports", no "turning
Code:
for ( int i = 0; i < <array>.length; i++ )
loops into
Code:
for ( <member> : <array> )
, no changing of variable/function name, etc, etc, etc.
This. Patch. Is. Only. Whitespaces.

There are other things I want to do, like adding generics to a lot of raw types (and even then, those have no impact at run-time whatsoever), but I didn't include them, specifically to make it so that there would be no reason to not accept this patch other than "one of the things I did was not the style we want".

OK. I think this patch has a perhaps infinitesimal but certainly non-zero probability of introducing a bug. I think there is no benefit to this patch. Thus a risk adverse cost benefit analysis says don't do it. If it ain't broke then don't fix it.

The fact that you did it by hand does not change the possibility of an unintended side effect for me. I could even argue that using automated tools to make a similar patch was more beneficial because the formatting would end up being consistent across the codebase.

But all I am really saying is that I do not want to commit this patch. If someone else with commit access wants to than I will agree to disagree but that's it. I might even begrudgingly admit in a few months that my fears were unfounded.
 

MCroft

Developer
Staff member
gonna jump on the bandwagon and say that I prefer incremental fixes with other changes. It absolutely leads to inconsistency between files, but it means that the scope of the change is such that it's at least getting developer testing and maybe review before being integrated.

When you fixed the https headers case issue, and we had attention to it specifically, I updated that particular file to use a case statement instead of a string of "if" statements. When I worked on the maximizer, I changed a bunch of for loops into enhanced for loops. Some files haven't changed since Java 1.6 and maybe Java 1.5. Some things aren't wrong (or weren't wrong back then), but could be improved.

If we had an amazing set of thorough unit tests, I might feel different, but I'm not inclined to adopt this as "the change I have to support if something is broken" by committing it.

I don't always agree with Frono about what shouldn't be done, but in this case, I think these changes would be best attached to desirable content and in smaller, bite-size, fixable-if-something-isn't-right chunks.
 
Top