Skip to content

Conversation

@subhramit
Copy link
Member

Use IntelliJ reformat with our config xml

Steps to test

Mandatory checks

  • I own the copyright of the code submitted and I license it under the MIT license
  • [/] I manually tested my changes in running JabRef (always required)
  • [/] I added JUnit tests for changes (if applicable)
  • [/] I added screenshots in the PR description (if change is visible to the user)
  • [/] I described the change in CHANGELOG.md in a way that is understandable for the average user (if change is visible to the user)
  • [/] I checked the user documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request updating file(s) in https://github.com/JabRef/user-documentation/tree/main/en.

@subhramit subhramit marked this pull request as draft September 13, 2025 11:54
@subhramit
Copy link
Member Author

Lol this was more of a disaster than I presumed

@koppor
Copy link
Member

koppor commented Sep 13, 2025

Currently compile errors - I don't understand why they are introduced. Maybe, someone can look based on main, why some formatting changes lead to compile errors:

Compiling with JDK Java compiler API.
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/entry/field/StandardField.java:198: error: name() in StandardField cannot override name() in Enum
    public String name() {
                  ^
  overridden method is final
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/entry/types/StandardEntryType.java:54: error: name() in StandardEntryType cannot override name() in Enum
    public String name() {
                  ^
  overridden method is final
warning: unknown enum constant Id.CLASS
  reason: class file for com.fasterxml.jackson.annotation.JsonTypeInfo$Id not found
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/logic/layout/format/NameFormatterPreferences.java:12: error: constructor is not canonical, so it must invoke another constructor of class NameFormatterPreferences
    public NameFormatterPreferences(List<String> nameFormatterKey, List<String> nameFormatterValue) {
           ^
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/logic/protectedterms/ProtectedTermsPreferences.java:14: error: constructor is not canonical, so it must invoke another constructor of class ProtectedTermsPreferences
    public ProtectedTermsPreferences(List<String> enabledInternalTermLists,
           ^
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/logic/util/UnknownFileType.java:11: error: constructor is not canonical, so it must invoke another constructor of class UnknownFileType
    public UnknownFileType(String... extensions) {
           ^
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/entry/field/InternalField.java:83: error: name() in InternalField cannot override name() in Enum
    public String name() {
                  ^
  overridden method is final
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/entry/field/BiblatexSoftwareField.java:63: error: name() in BiblatexSoftwareField cannot override name() in Enum
    public String name() {
                  ^
  overridden method is final
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/entry/field/AMSField.java:51: error: name() in AMSField cannot override name() in Enum
    public String name() {
                  ^
  overridden method is final
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/entry/field/BiblatexApaField.java:65: error: name() in BiblatexApaField cannot override name() in Enum
    public String name() {
                  ^
  overridden method is final
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/entry/field/SpecialField.java:93: error: name() in SpecialField cannot override name() in Enum
    public String name() {
                  ^
  overridden method is final
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/entry/field/IEEEField.java:39: error: name() in IEEEField cannot override name() in Enum
    public String name() {
                  ^
  overridden method is final
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/entry/types/BiblatexSoftwareEntryType.java:27: error: name() in BiblatexSoftwareEntryType cannot override name() in Enum
    public String name() {
                  ^
  overridden method is final
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/entry/types/SystematicLiteratureReviewStudyEntryType.java:25: error: name() in SystematicLiteratureReviewStudyEntryType cannot override name() in Enum
    public String name() {
                  ^
  overridden method is final
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/entry/types/IEEETranEntryType.java:27: error: name() in IEEETranEntryType cannot override name() in Enum
    public String name() {
                  ^
  overridden method is final
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/entry/types/BiblatexApaEntryType.java:28: error: name() in BiblatexApaEntryType cannot override name() in Enum
    public String name() {
                  ^
  overridden method is final
/home/runner/work/jabref/jabref/jablib/src/main/java/org/jabref/model/openoffice/rangesort/RangeSort.java:66: error: constructor is not canonical, so it must invoke another constructor of class HolderComparatorWithinPartition
            private HolderComparatorWithinPartition(XText cmp) {
                    ^

@koppor
Copy link
Member

koppor commented Sep 13, 2025

This is an alternative to JabRef#717

@@ -1,4 +1,4 @@
///usr/bin/env jbang "$0" "$@" ; exit $?
/// usr/bin/env jbang "$0" "$@" ; exit $?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code reformatting without adding new statements violates best practices. The change only adds a space after '///' which is purely cosmetic and doesn't improve functionality.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG - the code formatter -- not that important - we could also remove that line

But maybe, we add this to exceptions

@calixtus
Copy link
Member

image

Chris just recently fixed this. Please check if you have the very latest intellij config configured.

@koppor
Copy link
Member

koppor commented Sep 13, 2025

The fix of chris was #13817

@calixtus
Copy link
Member

image

Seems weird putting enums at the end of a file

jabgui/src/main/java/org/jabref/Launcher.java

@calixtus
Copy link
Member

image

No space between last variable declaration and method name?

@calixtus
Copy link
Member

@calixtus
Copy link
Member

Deliberately inserted newlines for readability should somehow be kept:
jabgui/src/main/java/org/jabref/gui/LibraryTab.java

image

@calixtus
Copy link
Member

Incorporated file from another project with their own copyright header.
image

@@ -1,4 +1,4 @@
///usr/bin/env jbang "$0" "$@" ; exit $?
/// usr/bin/env jbang "$0" "$@" ; exit $?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code reformatting was done without adding new statements. The addition of a space after '///' is purely syntactical and violates the code reformatting policy.

@@ -1,4 +1,4 @@
///usr/bin/env jbang "$0" "$@" ; exit $?
/// usr/bin/env jbang "$0" "$@" ; exit $?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code reformatting without adding new statements violates the principle of meaningful changes. The addition of a space after '///' is purely cosmetic and doesn't improve functionality.

@@ -1,4 +1,4 @@
///usr/bin/env jbang "$0" "$@" ; exit $?
/// usr/bin/env jbang "$0" "$@" ; exit $?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments should add new information and not just restate what's obvious from the code. The shebang line is self-explanatory and doesn't need additional comment formatting.

Comment on lines +218 to +219
case PROCESSING ->
notifications.add(new Notification(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code reformatting with changed indentation is purely cosmetic and doesn't introduce new functionality. Such changes should be combined with actual code modifications.

Comment on lines +12 to +13
public record Notification(
String title,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code was reformatted without adding new statements. According to guidelines, code should not be reformatted only for syntax reasons without adding new functionality.

Comment on lines +44 to +45
case ONLY_ABBREVIATED,
BOTH:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is being reformatted without adding new statements, which violates the principle of not reformatting code solely for syntax reasons without functional changes.

@koppor koppor mentioned this pull request Sep 13, 2025
1 task
private void cleanup(BibDatabaseContext databaseContext, CleanupPreferences cleanupPreferences) {
// undo granularity is on entry level
NamedCompound ce = new NamedCompound(Localization.lang("Cleanup entry"));
// undo granularity is on entry level
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment merely restates what is obvious from the code and variable naming. It does not provide additional information about reasoning or implementation details.

Comment on lines +57 to +58
case EntryChange entryChange ->
new EntryChangeDetailsView(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code reformatting appears to be purely cosmetic without adding new statements. The indentation and line breaks were modified without functional changes, which violates the reformatting policy.

@calixtus
Copy link
Member

grafik

@calixtus
Copy link
Member

grafik

@calixtus
Copy link
Member

grafik

@calixtus
Copy link
Member

@subhramit
Copy link
Member Author

@koppor what do we do about these?
Git revert and do another without *.fxml?
Manually reverting each will take years

@calixtus
Copy link
Member

Is @subhramit hidden as commit author to avoid git history misleading bc just reformatting?

@calixtus
Copy link
Member

@subhramit
Copy link
Member Author

subhramit commented Sep 13, 2025

grafik Can this be done with checkstyle? Empty line at the end of the comment

With checkstyle - yes, with intellij auto format configuration - no
@koppor would take a call on whether we want more warnings "after" an auto-format is done

However, I think in these cases, the formatter just moved */ to the next line, as there was already a * in the line (treating it as a valid comment line) (intended behavior - in a way, so needs manual removal).

@calixtus
Copy link
Member

grafik

@calixtus
Copy link
Member

grafik

Needs probably some thought

@calixtus
Copy link
Member

grafik

@koppor
Copy link
Member

koppor commented Sep 13, 2025

Is @subhramit hidden as commit author to avoid git history misleading bc just reformatting?

Yes --> #13881

@calixtus
Copy link
Member

grafik

Deliberately set apart

@calixtus
Copy link
Member

grafik

@koppor
Copy link
Member

koppor commented Sep 13, 2025

grafik

This is wrong JavaDoc by authors - needs manual fix.

@koppor koppor mentioned this pull request Sep 13, 2025
1 task
@calixtus
Copy link
Member

I like this very much:

grafik

@calixtus
Copy link
Member

grafik grafik

@calixtus
Copy link
Member

grafik

@calixtus
Copy link
Member

grafik grafik

@koppor koppor mentioned this pull request Sep 13, 2025
1 task
@calixtus
Copy link
Member

This made me laugh a bit:
grafik

@calixtus
Copy link
Member

@calixtus
Copy link
Member

grafik

@subhramit
Copy link
Member Author

No lines between methods? jablib/src/test/java/org/jabref/logic/citationkeypattern/BracketedPatternTest.java

grafik

That's gap between method and csvsource, not method and method

@koppor koppor mentioned this pull request Sep 13, 2025
1 task
@koppor
Copy link
Member

koppor commented Sep 13, 2025

grafik

This is minor, isn't it? -> should be converted to markdown in all cases?

@koppor koppor mentioned this pull request Sep 13, 2025
1 task
@koppor
Copy link
Member

koppor commented Sep 13, 2025

grafik

Deliberately set apart

One could introduce <p> if this is inteded. Not a show stupper

@koppor
Copy link
Member

koppor commented Sep 13, 2025

grafik

Reason for records reformatting: We began to use records, but did not update our code style.

Fixed at #13889

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dev: code-quality Issues related to code or architecture decisions project: jabcon status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants