signum-node icon indicating copy to clipboard operation
signum-node copied to clipboard

Code Formatting Guide Needed

Open damccull opened this issue 4 years ago • 3 comments

As seen in https://github.com/signum-network/signum-node/pull/469#discussion_r594667678 there exists a need for a formatting guideline or requirements document. I suggest basing the formatting on the most popular (or official, if any exists) java formatting standards and creating a FORMATTING.md or section of CONTRIBUTING.md that explains how formatting should be accomplished in this project.

Also, many IDEs have the ability to read auto-formatting rules from a file included in the project directory. Consider including formatting rules files for the most popular IDEs that support this option.

Checking for proper formatting should be done with an automated linter, if possible, that runs in a github workflow on every PR submission and update to ensure formatting is correct.

Here is a good example of a github action that can be deployed to automatically format the code into Google Java Format https://github.com/marketplace/actions/google-java-format. Not saying that's what we should use, but it's a good example. This completely automates the formatting and doesn't care what developers do in their own code. The --dry-run option is interesting as it does not actually commit the formatting changes. It could possibly be used to simply check for proper formatting and then fail the PR's workflow run if the formatting isn't correct.

damccull avatar Jul 18 '21 01:07 damccull

I think this is a good idea. There should be at least a minimal standard coding definition. But we have to think about the existing code and how it should be treated. if there will be a new definition which is not compatible with the existing code there has to be a guideline how to handle this. Maybe the complete existing code needs to be automatically reformatted once in that case (e.g. via your proposal https://github.com/marketplace/actions/google-java-format).

For the IDE's and Google Java Format (which you suggested) there are plugins existing for IntelliJ IDEA and Eclipse which i would say are widely in use.

Maybe we could create a fork or a branch and test the formatting rules / output for the existing code? The project used code generators like jOOQ which didn't followed the same coding style found in other places. So there will be a lot of changes.

Maybe some of the main devs like jj can give us some feedback on this. We could at least give it a try and make a proposal.

th1nkNXT avatar Aug 13 '21 19:08 th1nkNXT

@damccull I created a test branch: https://github.com/th1nkNXT/burstcoin/tree/feat/%23525

I added the Google Java Format Action to the jobs of the build workflow as a previous job before the build (on "push"). There are some Java related warnings but that needs to be addressed by the maintainer of the GitHub action and those are not relevant for the successful result of the action.

As i am using Eclipse i tested with https://github.com/google/styleguide/blob/gh-pages/eclipse-java-google-style.xml and activated the SaveActions but the result is not exactly the same as of the Google Java Format Action. Maybe the config file for Eclipse is too old or it is a problem in general. That means, every push will lead to a new commit related to the Google Java Format Action in my case.

As you can see (comparing the branch with main) the formatting has a huge impact on the current code. In a best case scenario there are no pending branches / pull requests when activating this (to avoid a large number of conflicts).

What do you think? How we should proceed?

th1nkNXT avatar Aug 20 '21 23:08 th1nkNXT

That sure is a lot of changes, but my opinion on when to format it: ASAP. Maybe put out a notice on the discord and as the top section of the README.md file of the change and then require branch authors to make the appropriate changes to their commits before merging them.

I will qualify ASAP with this: The auto-format job and the IDE autoformat command should match the formatting rules chosen, so that either one should push a properly formatted commit.

If an IDE-side format is not possible, then contributors should be asked to allow the formatting job to run on their own forks, then to squash the format job back into their own commits before sending a PR. That will ensure proper formatting server side, though at the expense of a bit of extra work for the contributor.

The best option is to get both commit-job-format and IDE formatting to match as closely as possible.

damccull avatar Aug 26 '21 23:08 damccull