ATTENTION: commit to fix formatting of openmrs-core

Hi there!

unfortunately code that gets commited to openmrs-core is often not formatted according to our OpenMRSFormatter.xml. Committers don’t run the maven build command before creating a PR, we had no checkstyle in place that would find such style violations so code was merged as is. Due to that the code is messy. There are even files with spaces instead of tabs, different levels of indentation, lines longer than our max line setting, …

You can easily see for yourself with for example in Eclipse running the formatter on the packages:

Example

Such a commit would touch a ton of files.

I would really like to create a commit that fixes formatting for all files once and for all but I am unsure if the consequences are too annoying. Will this be too much pain for backporting fixes? @dkayiwa

One problem of massive formatting changes that annoys me, is that you cannot view annotations to see who/why/when modified/inserted lines. Usually it shows @burke as responsible because massive changes on line endings CR/LF to LF.

@teleivo would that be a problem if we also back ported the formatting? :slight_smile:

but if the commit clearly says for example ‘automated formatting commit’ you’ll see this in the history and can simply forget about this one commit, no?

then no, but maybe too much work?

I have not tried to estimate how much work would be involved. Would you like to try it out? :slight_smile:

Not at the moment :joy: I still have a lot of hours of findbugs and codacy cleanups ahead :joy_cat:

History is fine, I mean Team -> Show annotations.

It should look like this (SVN):

But it looks like this:

EGit does not show commit’s author unless you move the cursor over.

as a little side note, @lluismf I’m sure you have some nice Eclipse tricks up your sleeve (things that make your dev workflow easy, plugins, anything) :wink: if you have something you think is worth sharing could you add it here: https://wiki.openmrs.org/display/docs/Developer+How-To+Use+Eclipse+Guide the page would really benefit some updates.

I’m not quite sure what the right answer is here. Code that isn’t properly formatted annoys me, but autoformatting can make merge requests impossible to handle, and I do find the “blame annotation” that @lluismf mentions very helpful at times.

I would tend to favor maintaining history vs auto-formatting.

Take care, Mark

I think although by default we usually annotate most recent revision of the file but there is a capability to annotate previous revisions (at least as far as intelJIDEA is concerned). As much as it would be annoying to have to do that, I do believe it is worth cleaning up the mess if the opportunity has presented itself. I personally like clean code that adhere to established standards and that sometime may come with cost or implications that we may not like much.

Besides the formatted files will keep changing with time, so eventually you won’t see only those annoying formatting annotations.