Don't return null in JobManager::createMonitor

[fedejeanne]

See 9c3525c#r140592763

Revert unintentional change made in 9c3525c and don't return null when creating a progress monitor. This is part of the contract of the method.

Contributes to #664

[github-actions]

Test Results

   639 files  ±0     639 suites  ±0   38m 48s ⏱️ - 1m 57s
 3 946 tests ±0   3 903 ✅  - 21   22 💤 ±0  14 ❌ +14  7 🔥 +7 
12 444 runs  ±0  12 262 ✅  - 21  161 💤 ±0  14 ❌ +14  7 🔥 +7 

For more details on these failures and errors, see this check.

Results for commit 84578cc. ± Comparison against base commit 6de5471.

[HeikoKlare]

Please avoid unrelated changes or ensure that they are correct. In this PR, there are conditions that have been merged such that the associated comments are not placed at the conditions they refer to anymore. Can you please fix that?

[iloveeclipse]

In this PR, there are conditions that have been merged such that the associated comments are not placed at the conditions they refer to anymore. Can you please fix that?

Yes, this is very sensible & hard to understand code code. Such changes shouldn't be part of any unrelated fix. Please revert them.

[fedejeanne]

In this PR, there are conditions that have been merged such that the associated comments are not placed at the conditions they refer to anymore. Can you please fix that?

Yes, this is very sensible & hard to understand code code. Such changes shouldn't be part of any unrelated fix. Please revert them.

Hm, it was probably the cleanup "Single 'if' statement rather than duplicate blocks that fall through", which is part of the Eclipse [built-in] profile. I'll take a look at it tomorrow and see if it's ok to disable it.

Thank you for noticing, I reverted the changes in #1285 (since this PR is already merged)

[HeikoKlare]

Hm, it was probably the cleanup "Single 'if' statement rather than duplicate blocks that fall through", which is part of the Eclipse [built-in] profile. I'll take a look at it tomorrow and see if it's ok to disable it.

Cleanup is not performed automatically, so it probably was the save actions. The save actions have that option enabled and that is intended (see #1001) as the option usually makes sense.
Still, there may be some code places that do not conform to that pattern (there were a bunch in #1113), but from my experience over the last months (since #1001), they are rather rare. In case the save action produces changes in existing code (like here), you can decide whether you want to incorporate the changes (probably adapting them to to be proper) or whether you do not want to commit them.

[fedejeanne]

Has someone run a cleanup on several files yet or was that never the intention? (see #1001 (comment))

[HeikoKlare]

Has someone run a cleanup on several files yet or was that never the intention?

I have not noticed that anyone did that. Note that this would require to first thoroughly align all cleanup configurations with the save action configurations, as they need to be consistent. Not sure whether that's worth the effort.

[fedejeanne]

👍
Probably not worth the effort since the very things that happened in this PR can happen again which means the reviews need to be really thorough (and the changes will probably involve thousands of lines)

[vogella]

Applying useful batch clean-ups is something the PMC encourages. If done they should be split is small PR to enable manual checks. That could be project or package related

Note, in the past the Jdt cleanups not yet used in platform used to be buggy. Most of them have improved and stabilised over time based on our usage of them in platform and pde

[fedejeanne]

Apologies in advance for the lenghty response. We can move this to a discussion if you guys think it would be better.

@vogella even though I'm a big fan of automatic Clean ups (they achieve "homogeneous" code, they help prevent "indentation bugs", etc), there are some considerations to it, like:

• How to do it? Per package, per class (i.e. how "big" should a PR be)?
• Where to do it? Everywhere or only components that need to be changed (or that have been recently changed) anyway?
• Is it a problem that the commit history gets "polluted" by such commits? This one really annoys some colleagues because, and I quote, "it's unnecessarily complicated to see who wrote a faulty if-statement if the history shows the one that added the bracket ({) at the end of the line"

Are there any general guidelines/recommendations to do the clean-ups that I can follow? Or should we start writing some as we go along?

As a side note: A clean up in JobManager alone would create 133 changes. Most of them are adding brackets to one-line-blocks (if-blocks) and there is also the changes that I reverted in this PR, which need to be found and avoided. The good thing is: now that we know that these changes may occur, I could just propose an improvement to the clean-up rule so it does not apply if the if-statements have comments in-between.

[HeikoKlare]

The discussion becomes far broader than it was (or needs to be) in the context of this PR. Of course we can discuss about general cleanup, but that's competely unrelated to this PR, as in this case no cleanup was performed at all. I just want to repeat my initial statement:

Cleanup is not performed automatically, so it probably was the save actions.

The original issue was about executed save actions and the risk of having further changes because of save actions being applied unnoticed. To prevent that, it is not necessary to apply the concurrently configured cleanup options, as they will apply far more changes than requires to prevent accidental changes by save actions. In this case, it's about the mentioned 133 changes in JobManager via cleanup vs. the few ones that have been applied via the save actions). That's why I wrote:

Note that this would require to first thoroughly align all cleanup configurations with the save action configurations, as they need to be consistent.

So to summarize:

• "Cleaning up" code in the sense that all automated save action will become no-ops could be reasonable.
• Simply applying clean-up rules to all code is something I would not be in favor of.

[vogella]

The pmc rule is that clean-ups are fine (in whatever scope the committer finds useful). Git commit history "pollution" is no reason not to do it.

In the past, I applied code clean-ups to all code:

• if the result is supposed to be faster (jdt performance clean-ups)
• if the code is easier to read or shorter
• if not applying it would result in "unrelated changes" due to save actions
• to test the jdt clean-ups and result issues with them
• make code more modern and hence more attractive to new contributors

Any change should be reasonable small so that is can be reviewed by others if they want and should only contain one type of clean-up. Additional manual changes should be avoided or clearly marked via the commit message.

Search the Git history for clean-ups to see examples.

[Bananeweizen]

If we are in a side discussion anyway: Please be aware that the project settings are often not fully maintained. Let me explain what I mean by that, taking the core.search project of this PR as example.

• Quite often, *.prefs files are not checked in. And then every change in workspace preferences done by a developer will lead to his/her IDE behaving differently. In this PR, the project has all prefs files.
• When prefs files are checked in, they are often not updated to contain newly added key/value pairs. To see the effect, take an arbitrary project with prefs under version control, then open project properties, show every sub page of the tree, and finally hit "Apply and Close". That will serialize all prefs with all key/value pairs. For the jdt.core prefs of core.search that adds 80 new key/value pairs, mostly related to the formatter. All the values for these keys are still taken from workspace preferences.
  
• Some of the properties files have their own versioning, which controls which values are actually read in the Java code. Not incrementing that version may then lead to ignoring all keys that were introduced after that version number, even though the keys are in the prefs files:
  

At work I've therefore come to a solution where I regularly overwrite all prefs files of all projects with a centrally managed copy. And that copy is refreshed after every Eclipse release, to make sure that all key/values and version numbers are available. Not sure if some more strict management can and should be applied here, too. If I remember right, @merks also has some kind of template mechanism in Oomph to copy these preferences from one project to all others, but I might be wrong there.

[HeikoKlare]

@Bananeweizen Thank you for the great input. That's a nice summary of problems w.r.t. preference file maintenance. I like the idea of an automated preference file update mechanism and would be in favor of having that implemented. Since the list of projects is quite stable in the Eclipse platform, preference files not being checked in does not seem to be a relevant issue to me. But "incomplete" and/or out-of-date preference files will always be an issue. As far as I find the time for that, I currently try to bring all platform projects (including platform, platform.ui and maybe SWT) into a state where the code conforms to the save action definition that we have deployed to all projects in the platform repo. If meanwhile someone is interested in providing an automatism to keep the default preference files up-to-date and deploy them to all projects (and if no one objects on the idea), I would appreciate that.