I am trying to make my first Pull Request to OpenMRS and am hitting code formatting obstacles.
I’m amending existing code, but I want to avoid changing its formatting since this could confuse the reviewer. I’m using the OpenMRS formatter (in IntelliJ).
The issues are as follows:
When I apply code formatting to ensure my changes adhere to the standard, existing code is re-formatted. Unpicking formatting changes from my changes is an error-prone process.
For my new code, the OpenMRS formatter imposes changes which Codacy rejects when I instigate a Pull Request. The formatter places a “catch” on a different line to the preceeding “}”, but this upsets Codacy.
Also, the OpenMRS formatter doesn’t alert me to issues which Codacy will have with my code. This relates to required whitespace, line length, method of line-splitting (a “+” must be at the start of a line), empty catch blocks, etc.
Every time I resubmit a Pull Request this kicks off a lengthy process on the CI server, which eventually comes back with unexpected problems. So my question is - what do people do in practise? Do people run Codacy locally, just for the changed files? I have a feeling I am missing the point somehow.
(btw I am aware of the dangers of “religious” debates about code-formatting - my view is its only important to adhere to an agreed standard for a project).
sorry for the late reply and thanks for sharing this!!
I know the setup can use some more work, problem is IDEs move fast, people use different ones, the maven formatter we use is no longer supported and am not sure yet which alternative to use to make this easier for everyone.
that is the point of the formatter it should change existing code. why would you unpick changes? do you mean because an entire class was not formatted and so it “hides” your changes?
I also encounter that a lot of files are not formatted as we would like. We just decided not to format the entire project and commit the changes that since some pointed out that it would make the git history harder to read. So we just have to clean up bit by bit. But dont worry. In my point of view its ok and good to also commit those formatting changes in files you changed.
I assume that lluismf_2 has different rules in place which is causing the trouble. lluismf_2 should not interfere with the PRs of openmrs-core, we need to investigate this! Thanks
did you import the formatter in your IDE and use it? Or run mvn clean package locally? Than codacy should not complain. Again 2) needs to be looked at because this seems to enforce the wrong rules.
We could also let the build fail on formatting issues but I think that would be to religious
And also reviewers can simply merge your changes despite codacy. I know the setup is not yet easy and I really want to improve this as I think we should focus on the problems and the code and not formatting
Thanks a lot for sharing and Ill get back to you if I figure out 2)
re 1), the problem with the formatter was that it made many changes to existing code, making it problematic for a reviewer to see my changes easily. I think it would also make it more difficult to read the change history of the file once the changes are committed.
re 3), I initially used both the openmrs formatter for IntelliJ, and ran mvn clean package. I then hit multiple formatting problems with Codacy when I submitted the PR.
Eventually I started over, going back to the original code, and cut and paste in my changes. I then deleted the original PR and submitted a new one, without having done any formatting checks, so as to let Codacy tell me what it didn’t like. I altered the formatting accordingly and resubmitted.
I think it is wise not to change formatting for existing files, I have been on projects where it is hard to read change histories of files because you have to pick out the real changes from the cosmetic.
If I recall correctly, I just clicked on the link which appeared automatically when I submitted the PR to core. There were the usual code coverage and test results, with links, plus this link to the Codacy report.
I asked for a code review and @dkayiwa replied, asking me why I had changed the formatting, which prompted this discussion.
I wasn’t aware that the Codacy URL pointed to a lluismf_2 page until you pointed it out. It was a few weeks ago, so I may be forgetting details, though…
and thanks for sharing this!!
