Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to palantir-java-format #663

Closed
wants to merge 3 commits into from
Closed

Conversation

koppor
Copy link
Owner

@koppor koppor commented Oct 28, 2023

I am so fed up that

Thus, I am looking for good autoformatting tools. I thought Google's Java Code Style would be great. However, 4 spaces need extra configuration. Moreover, the project seems to be unmaintained currently. Luckily, I found a maintained alternative: palantir-java-format

The diffs look good. Only three drawbacks:

  • @FXML is placed before the declaration (because all annotations are placed the line before)
  • this( reads strange with the 2x4 space indent next line
  • XML and CSS is also reformatted

For me these are no real drawbacks or show stoppers.

REgarding migration: One needs to switch away from JabRef code style to "Default". However, I think, this is manageable as the active contributors can be informed - and the "drive-by" contributors are setting up their IDE before their contribution.

image

(GJF was tried out at #661)


Open issues:

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked 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 to the documentation repository.

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Collaborator

k3KAW8Pnf7mkmdSMPHz27 commented Oct 29, 2023

My only problem with it thus far has been,

  1. they update the style a bit faster than I'd like,
  2. I mainly use Maven 😢
  3. The marketplace plugin hasn't always been compatible with the most recent IntelliJ IDEA versions

I like the look it enforces, and it has some customizability with the Checkstyle rules. I'll try setting it up for IntelliJ in Windows based on this branch.

Copy link

Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues.

More information on code quality in JabRef is available at https://devdocs.jabref.org/getting-into-the-code/development-strategy.html.

Copy link

Your code currently does not meet JabRef's code guidelines. We use OpenRewrite to ensure "modern" Java coding practices. The issues found can be automatically fixed. Please execute the gradle task rewriteRun, check the results, commit, and push.

You can check the detailed error output at the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".

@koppor
Copy link
Owner Author

koppor commented Nov 17, 2023

As alternative, I checked spring-java-format, but it provides bad wrapping:

     /**
-     * Add X11 clipboard support to a text input control. It is necessary to call this method in every input where you
-     * want to use it: {@code ClipBoardManager.addX11Support(TextInputControl input);}.
-     *
-     * @param input the TextInputControl (e.g., TextField, TextArea, and children) where adding this functionality.
-     * @see <a href="https://www.uninformativ.de/blog/postings/2017-04-02/0/POSTING-en.html">Short summary for X11
-     * clipboards</a>
-     * @see <a href="https://unix.stackexchange.com/questions/139191/whats-the-difference-between-primary-selection-and-clipboard-buffer/139193#139193">Longer
+     * Add X11 clipboard support to a text input control. It is necessary to call this
+     * method in every input where you want to use it:
+     * {@code ClipBoardManager.addX11Support(TextInputControl input);}.
+     * @param input the TextInputControl (e.g., TextField, TextArea, and children) where
+     * adding this functionality.
+     * @see <a href=
+     * "https://www.uninformativ.de/blog/postings/2017-04-02/0/POSTING-en.html">Short
+     * summary for X11 clipboards</a>
+     * @see <a href=
+     * "https://unix.stackexchange.com/questions/139191/whats-the-difference-between-primary-selection-and-clipboard-buffer/139193#139193">Longer
      * text over
-    public ArgumentProcessor(String[] args,
-                             Mode startupMode,
-                             PreferencesService preferencesService,
-                             FileUpdateMonitor fileUpdateMonitor,
-                             BibEntryTypesManager entryTypesManager) throws org.apache.commons.cli.ParseException {
+    public ArgumentProcessor(String[] args, Mode startupMode, PreferencesService preferencesService,
+            FileUpdateMonitor fileUpdateMonitor, BibEntryTypesManager entryTypesManager)
+            throws org.apache.commons.cli.ParseException {

@koppor
Copy link
Owner Author

koppor commented Nov 17, 2023

Note that spring java format uses tabs, because of good reasons:

@koppor
Copy link
Owner Author

koppor commented Nov 17, 2023

prettier-java does a great job in line wrapping.

palantir output:

        action.getKeyBinding().ifPresent(keyBinding -> keyBindingRepository
                .getKeyCombination(keyBinding)
                .ifPresent(combination -> setAccelerator(combination)));

prettier output:

        action
            .getKeyBinding()
            .ifPresent(keyBinding ->
                keyBindingRepository
                    .getKeyCombination(keyBinding)
                    .ifPresent(combination -> setAccelerator(combination))
            );

I like prettier more, because

  • the subject is "always" in the first thing in the line.
  • it also formats the imports

I'll file another PR. Let's see, how it works out ^^

@koppor
Copy link
Owner Author

koppor commented Nov 18, 2023

The prettier-java output was "random", because line wrapping was at 80 chars... If set to 100 or 120 the wrapping is worse.

@koppor koppor changed the title Switch to plantir-java-format Switch to palantir-java-format Nov 27, 2023
@koppor koppor added the freeze label Nov 27, 2023
@koppor
Copy link
Owner Author

koppor commented Nov 27, 2023

Blocked by palantir/palantir-java-format#935

@koppor
Copy link
Owner Author

koppor commented Dec 8, 2023

Is it ehtically OK to use Palantir - even if it is "the best candidate"?

https://www.heise.de/news/Trotz-Bedenken-Bayern-testet-Palantir-Software-mit-echten-Personendaten-9545037.html

umstrittene[s] US-Unternehmen[...] Palantir

@koppor koppor mentioned this pull request Jan 9, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants