search

katharsis-project / katharsis-framework

Katharsis adds powerful layer for RESTful endpoints providing implementenation of JSON:API standard
http://katharsis.io
Apache License 2.0
135 stars 65 forks source link

Updating CONTRIBUTING.md with more developer guidelines #243

Open Ramblurr opened 7 years ago

Ramblurr commented 7 years ago

Background

The current CONTRIBUTING document is in draft state and needs to be updated.

Also I've noticed a discrepancy in code-style across all the modules. There is a wide mix of tabs and space. The import order varies wildly. There are some other few discrepancies as well.

These discrepancies will only continue to grow the more developers we add to the project. The primary reason to use a single code style is to reduce change noise.

Without a single codestyle pull requests by default will have lots of noise in the form of import order changes, whitespace changes, etc. Fixing this noise is difficult as it involves fighting your IDE's settings.

Proposal

This can all be nipped in the bud by specifying an official Katharsis project formatter for Eclipse and IntelliJ.

Most of the developers are already using the same style, in theory, so there is no reason to change. The only settings that need tweaking or whitespace and import order.

While at it we can spruce of the Contribution guidelines and specify default ISSUE and PR templates to encourage correct and useful contributes from the community.

Tabs or Spaces

According to the Intellij "Problematic whitespace" checker, there are:

If we use tabs: 628 files that must be changed. If we use spaces: 506 files that must be changed.

I hope we can make a decision here that is fast and friendly.

Import order

I propose the following import order: preferences

This will keep the imports organized and clean.

Hosting the formatters

TODO

If everyon is in agreement then we can:

Ramblurr commented 7 years ago

I don't have an opinion on tabs or spaces, my vote is for whatever the consensus is.

remmeier commented 7 years ago

we may put the formatting into maven itself as a step. Eclipse, Intellj, etc. can be configured to look quite a like, but they still differ. Code formatter configurations for Eclipse and Intellij nevertheless would be helpful.

Ramblurr commented 7 years ago

@remmeier Any comment on tabs/space and import order?

It's true Eclipse and IDEA differ, but not on the most common source of PR noise: import order and whitespace. If lock down those settings, and then everyone runs the formatter on just the changed lines (intellij eclipse, then we'll be good. Actually... I will add these to the PR.

The configs I'm building are taken from the spring-data projects own config, they require it in their CONTRIBUTING.

remmeier commented 7 years ago

I prefer tabs. But I'm also able to sleep at nights if spaces are used.

remmeier commented 7 years ago

import order is ok for me, I'm not really looking at those.

chb0github commented 7 years ago

Spaces are more portable than tabs -- 4 spaces I think is the default IJ arrangement.

masterspambot commented 7 years ago

Let's don't start battle tabs vs spaces here guys! 🛩

Ramblurr commented 7 years ago

@masterspambot I totally agree, I don't want to start an argument, and based on the responses it doesn't seem like anyone is keen on starting it either. But there are different opinions, and we need to come to a consensus.

If we don't then the commits become more noisy as extraneous whitespace changes, or files start getting mixed tabs/spaces.

I'll give this another few days, then see what people have chosen, if there is no consensus I will count the total number of files with tabs and with spaces and choose the one with the largest.

Then I can upload the eclipse + IJ format configs.

mohlek commented 7 years ago

Personally I like spaces. Maybe its a good idea to look what IDE is used the most ( for this project ) and go with the "default" settings?!

masterspambot commented 7 years ago

Two cents from me. Instead of keeping IDE configs in code lets embed formatting/style checking in build process. What do you think about that? This would make us IDE agnostic and enforce code formatting style.

https://github.com/revelc/formatter-maven-plugin http://www.jillesvangurp.com/2015/01/29/enforcing-code-conventions-in-java/

Personally I am for tabs: http://softwareengineering.stackexchange.com/a/2037

remmeier commented 7 years ago

formatting in the build sounds perfect to me.

So far the the tabs seem to be favored. Not uncommon in the Java world I would say.

mohlek commented 7 years ago

http://www.jillesvangurp.com/2015/01/29/enforcing-code-conventions-in-java/

this article seems to cover everything we need

masterspambot commented 7 years ago

I am glad we came to agreement! 👏 👏

chb0github commented 7 years ago

One thing I don't want to get into is a lowest-common-denominator development environment where we use 80-character lines because someone has decided he likes to develop on old mainframes. I have 2x27" displays.

The tool in use doesn't matter too much to me.

BTW: are people really finding the coding format impossible to work with?

masterspambot commented 7 years ago

It is often seen in PR @chb0github... That people reformat code in there and whole file is included.

On Mon, 2 Jan 2017, 21:54 Christian Bongiorno, notifications@github.com wrote:

One thing I don't want to get into is a lowest-common-denominator development environment where we use 80-character lines because someone has decided he likes to develop on old mainframes. I have 2x27" displays.

The tool in use doesn't matter too much to me.

BTW: are people really finding the coding format impossible to work with?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/katharsis-project/katharsis-framework/issues/243#issuecomment-270019268, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjeAfXzORKpAtTR65Nudm5wEk3BsXsoks5rOWPugaJpZM4K--cj .

Ramblurr commented 7 years ago

Yes, like @masterspambot said the main problem with not having a project standard is that the PRs and commits are very very noisy as the different IDEs apply formatting to a file when changing only a few lines.

In the slightly worse case the dev might have the IDE to only apply formatting rules to lines they actually change, but this ends up with inconsistent styles as some lines are 80 chars with spaces and others are 100 with tabs and all { on new lines.

I'll setup that maven plugin and see if it behaves sanely.

(also, I'm for longer lines too.)