Code cleanup: spacing, tabs at ends of lines

[josephsl]

Hi,

Briefly discussed in #6589:

There are some instances where we have different spacing to denote boundaries between classes, as well as lines that only have tabs (and appears to be blank when it isn't). This became prominent during introduction of GUI Helper services.

Thus the following are proposed:

1. Locate lins that only have tabs and remove them.
2. Use two lines to denote boundaries between classes.
3. Other consistent style for blank lines.

@feerrenrut, @derekriemer and others, any thoughts?

Thanks.

[feerrenrut]

I can see how this is important, and its unfortunate that the blank lines weren't picked up before being merged, I'll try look out for this more carefully in the future. Generally I would try to avoid code changes just to fix formatting, because it doesn't provide functionality but adds noise to the file history and can cause merge conflicts. However if this improves the readability of the code, then that is valuable. If the changed lines are all blank or whitespace, then it is unlikely to cause problems with merges / file history.

[josephsl]

Hi, I’ll commit them in stages (used Notepad++ to locate these), then once the PR is approved and tested, you can try a squash merge. Thanks.

[jcsteh]

I'm not convinced this really makes a noticeable difference to code readability. If you were working on an affected area of code and wanted to fix something, that's fair enough. However, a PR to fix a lot of white space issues is going to be time consuming to review. Sure, you can easily check that it only touches white space, but if you actually want to verify it's doing the right thing with white space (which is the whole point of this issue), you'd have to closely scrutinise the entire diff. That is not currently an effective use of the limited time we have for code review.

[Adriani90]

This will be actually fixed interactively also through flake8 errors and warnings. @josephsl don't you think we could close this now?

[josephsl]

Hi, I’d still go through manual inspections if needed. Thanks.

[feerrenrut]

I don't think this is a good use of time, and only introduces noise into the history. As mentioned we have the linter to stop new issues from being introduced, and this should gradually affect older files too. Even if this was done by the community as a contribution, I don't think we could justify the time to review something like this.