Enforce LeftCurly rule #6452

koppor · 2020-05-08T23:33:26Z

This is a follow-up to #6283.

I open this PR just for documentation. No need to review.

While reviewing #6426, I found out, the contributors did not have setup the code style properly. Thus, I now put LeftCurly in place, which enforces that there are no statements directly appended after an if.

-public DoubleProperty widthProperty() { return widthProperty; }
+public DoubleProperty widthProperty() {
+    return widthProperty;
+}

I am aware that for simple values, this causes more lines without improving things. Nevertheless, I vote for consistent code.

The future should be to rewrite minor code style issues (unused imports, line breaks, ...) automatically. The current tool of choise is spotless. Also Google's refaster should be put in place. It has very advanced rewriting templates. For instance:

assertEquals(true, ...);

can be automatically rewritten to

assertTrue(...);

Siedlerchr · 2020-05-09T11:39:09Z

Argh, again some temporary issues:
> Unable to load Maven meta-data from https://repository.apache.org/snapshots/org/apache/logging/log4j/log4j-jcl/3.0.0-SNAPSHOT/maven-metadata.xml.

tobiasdiez

Does intellij and eclipse support automatic formatting according to this rule? Stricter code formatting rules are fine with me as long as I don't have to manually fix the formatting.

Moreover, some of the changes to the javadoc comments seem strange to me.

tobiasdiez · 2020-05-09T16:25:41Z

src/main/java/org/jabref/gui/fieldeditors/FieldEditorFX.java

-     * A value of 1 means that the editor gets exactly as much space as all other regular editors.
-     * A value of 2 means that the editor gets twice as much space as regular editors.
+     * <p>
+     * A value of 1 means that the editor gets exactly as much space as all other regular editors. A value of 2 means


Minor point: Did you do this manually or via your code editor? The previous version was better in my opinion.

InteliJ autoformat. I added an additional

It renders now as: (pressed Ctrl+Q)

Before, it rendered as

tobiasdiez · 2020-05-09T16:26:22Z

src/main/java/org/jabref/gui/maintable/MainTableColumnModel.java

@@ -99,7 +99,8 @@ public MainTableColumnModel(Type type, String qualifier) {
    }

    /**
-     * This is used by the preferences dialog, to initialize available basic icon columns, the user can add to the table.
+     * This is used by the preferences dialog, to initialize available basic icon columns, the user can add to the


same here, why the added line break?

tobiasdiez · 2020-05-09T16:27:39Z

src/main/java/org/jabref/gui/preferences/PreviewTabViewModel.java

-     * XML-Syntax-Highlighting for RichTextFX-Codearea created by (c) Carlos Martins (github: @cemartins)
-     * License: BSD-2-Clause
-     * see https://github.com/FXMisc/RichTextFX/blob/master/LICENSE and:
+     * XML-Syntax-Highlighting for RichTextFX-Codearea created by (c) Carlos Martins (github: @cemartins) License:


koppor · 2020-05-10T10:39:13Z

I pressed automatic reformat in IntelliJ. No manual changes.

If we do not want to have these changes, we need to modify our automatic formatting rules.

koppor · 2020-05-10T12:49:32Z

This is a step forward that we are back in 2015, where we enforced that all contributors pressed Ctrl+Alt+L (in IntelliJ) to ensure proper formatting of the whole file.

tobiasdiez · 2020-05-10T13:40:20Z

config/IntelliJ Code Style.xml

-  <option name="WRAP_COMMENTS" value="true" />
+<code_scheme name="JabRef" version="173">
+  <option name="RIGHT_MARGIN" value="140" />
+  <option name="WRAP_WHEN_TYPING_REACHES_RIGHT_MARGIN" value="true" />


I think we had a discussion about this some time ago and decided against having a hard line wrap (reasoning was "old school, everybody has wide monitors now anyway" if I remember correctly).

Yes, no hard-line wrap. I remember the discussion

I think, we did not adapt the config and we had no issues until I applied the automatic formatting today. Either none of use applied to automatic formatting before or was a wizzard in git gui only committing the reformat of code; not that of the comments.

I did the latter one often. However, currentyl, I believe, I cannot really explain to contributors that we have automatic tooling, but one cannot fully use it.

I changed the limit to 500 characters at 59938a9. This should be high enough.

Removed the hard limit as we have files with 510 or 5000 characters line length.

koppor · 2020-05-10T13:50:36Z

I changed the maximum columsn from 120 to 140 at 73f8ceb.

Google Java coding style: 100 characters. See https://qr.ae/pNynqB for a discussion. See also https://github.com/checkstyle/checkstyle/blob/4768470f54ec6f3031bd335157db9413c3906be9/src/main/resources/google_checks.xml#L45
ChecksStyle code: 100 characters: See https://github.com/checkstyle/checkstyle/blob/4768470f54ec6f3031bd335157db9413c3906be9/config/checkstyle_checks.xml#L182.
Sun: 80 characters. - This is the default of CheckStyle: https://checkstyle.sourceforge.io/config_sizes.html#LineLength

Discussions in the net

general discussion: https://dev.to/buinauskas/maximum-line-length-in-your-code-2ia4
soft limit 120 chars: https://www.php-fig.org/psr/psr-12/
more than 100 chars: https://hackernoon.com/does-column-width-of-80-make-sense-in-2018-50c161fbdcf6

koppor · 2020-05-10T15:12:43Z

Filed an issue to IntelliJ, because it automatically reformats the JavaDoc even if not told so

https://youtrack.jetbrains.com/issue/IDEA-240517

- Array initializers have to be separated by space - Space in single-line enums

koppor · 2020-05-10T17:34:06Z

Reformatted all files according to our IntelliJ formatting settings. I did not commit the change to one single line in the JavaDoc, but used the other changes.

Now all students can format the their contribution automatically.

For Eclipse, we need to check. Since these will be minor issues, I would propose to handle that as soon as an issue arises.

src/main/java/org/jabref/cli/JabRefCLI.java

# Conflicts: # src/main/java/org/jabref/gui/DialogService.java # src/main/java/org/jabref/gui/StateManager.java

* upstream/master: (50 commits) Keep group pane size when resizing window (#6180) (#6423) Changelog: Fix missing citation for biblatex-mla Update AUTHORS Check duplicate DOI (#6333) Fix missing citation for biblatex-mla Change EasyBind dependency (#6480) Add testing of latest dev version as mandatory Squashed 'src/main/resources/csl-styles/' changes from 5dad23d..586e0b8 Fix libre office connection and other progress dialogs (#6478) Fix clear year and month field when converting to biblatex (#6434) Add truncate as a BibTex key modifier (#6427) Add new authors (not all - they need more work) Remove empty line Add simple Unit Tests for #6207 (#6240) Enforce LeftCurly rule (#6452) Implement task progress indicator (and dialog) in the toolbar (#6443) Consider empty brackets Changelog update Added a test Fixed brackets in regular expressions ...

Enforce LeftCurly rule

2316530

koppor added the type: code-quality Issues related to code or architecture decisions label May 8, 2020

Fix checkstyle

7cc592b

tobiasdiez reviewed May 9, 2020

View reviewed changes

Add "including tests" to the Checkstyle configuration

4604c68

Change maximum columns to 140 characters

73f8ceb

tobiasdiez reviewed May 10, 2020

View reviewed changes

koppor added 2 commits May 10, 2020 15:46

Display "warning" line at columnn 120

061edc6

Add line length check (140 characters)

54c238c

koppor added 2 commits May 10, 2020 15:59

Also wrap single statements

10eeaa2

Hard limit for line length: 500

59938a9

koppor added 4 commits May 10, 2020 19:15

Reformat all code (and adapt codestyle to match existing code)

dfde19f

- Array initializers have to be separated by space - Space in single-line enums

Fix checkstyle.xml

30de8ad

Disable LineLength check

1d18d5c

Merge remote-tracking branch 'origin/master' into improve-checkstyle

fd543ea

Try to disable hard wrap

de6c1af

tobiasdiez reviewed May 12, 2020

View reviewed changes

src/main/java/org/jabref/cli/JabRefCLI.java Outdated Show resolved Hide resolved

koppor added 2 commits May 13, 2020 06:53

Merge remote-tracking branch 'origin/master' into improve-checkstyle

bb28373

# Conflicts: # src/main/java/org/jabref/gui/DialogService.java # src/main/java/org/jabref/gui/StateManager.java

Reformat JabRef CLI (rearrangte the . (dot))

a629720

koppor merged commit 4e220f6 into master May 13, 2020

koppor deleted the improve-checkstyle branch May 13, 2020 05:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce LeftCurly rule #6452

Enforce LeftCurly rule #6452

koppor commented May 8, 2020

Siedlerchr commented May 9, 2020

tobiasdiez left a comment

tobiasdiez May 9, 2020

koppor May 10, 2020

tobiasdiez May 9, 2020

tobiasdiez May 9, 2020

koppor commented May 10, 2020

koppor commented May 10, 2020

tobiasdiez May 10, 2020

Siedlerchr May 10, 2020

koppor May 10, 2020

koppor May 10, 2020

koppor commented May 10, 2020 •

edited

Loading

koppor commented May 10, 2020

koppor commented May 10, 2020

Enforce LeftCurly rule #6452

Enforce LeftCurly rule #6452

Conversation

koppor commented May 8, 2020

Siedlerchr commented May 9, 2020

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented May 10, 2020

koppor commented May 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented May 10, 2020 • edited Loading

Discussions in the net

koppor commented May 10, 2020

koppor commented May 10, 2020

koppor commented May 10, 2020 •

edited

Loading