Development: Modify the IntelliJ editor configuration to discourage star imports

[reschandreas]

Checklist

General

• This is a small issue that I tested locally in my IDE.
• Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
• I chose a title conforming to the naming conventions for pull requests.

Motivation and Context

We have multiple code scanning tools that complain about our usage of star imports or other formatting issues, making it hard to spot any real findings that should be taken care of.

Description

I exported the default editor configuration of Intellij and removed everything I could see that did not make sense for us. The only values that I changed is the usage of star imports to 1000 to prevent teamscale findings and coderabbit comments.
I do not intend to tell you how to format your code, I'd very much like this editor config to be a living document and be changeable by all. After all its just formatting and easily changeable.

Review Progress

Code Review

• Code Review 1
• Code Review 2

Summary by CodeRabbit

• Refactor
  • Updated the .editorconfig to improve code formatting standards, including line length and tab width adjustments.
• Chores
  • Added specific .editorconfig settings for Liquibase changelog files to prevent reformatting issues.
  • Included a new .editorconfig in the test resources to address potential formatting-related test failures.

[coderabbitai]

Walkthrough

The updates bring about stricter coding standards and formatting rules project-wide. Key changes include implementing a maximum line length and standardizing tab width. Moreover, adjustments to configurations for Java and Python files ensure consistent formatting. Newly added .editorconfig files in specific directories aim to prevent reformatting issues that could impact database migrations and test executions.

Changes

File Path	Change Summary
.editorconfig	Added max_line_length, set tab_width, modified ij_java_ and ij_python_ properties.
src/.../config/liquibase/changelog/.editorconfig	New file to prevent reformatting that could change checksums.
src/test/resources/.editorconfig	New file to set root and warn against reformatting that could break tests.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository from git and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[nityanandaz]

Thanks for handling the issue

[Hialus]

The amount of changes in this file is simply way too big to properly review them. I found at least one instance where it is not in line with our existing Spotless settings. For changes like this we would have to make sure that they are 100% in line with our other linter settings that we use. For this we would have to painstakingly go through each setting and compare them with Spotless (and possibly other linters)
There are several other existing differences between IntelliJ default settings and our Linter settings that I am aware of that likely are also included in the changes here. (e.g. double vs. single quotes in JS code is a common thing).

I think our priority should not be to enforce the default IntelliJ settings in our editorconfig, but to enforce our custom linter rules that differ from the default IntelliJ settings.
It is important to remember that the editorconfig is used when you reformat or generate code in IntelliJ. Our linters run as a pre-commit hook and would then complain about or revert differences.

[Hialus]

This should ideally be included in Spotless if possible. A quick search suggests that this should be possible.

[reschandreas]

I couldn't quickly find anything that suggests that spotless could do that, I think spotless only looks at one file at a time and does not know which classes of the package are actually used.

[reschandreas]

Hi @Hialus,
Thanks for the input.

1. I wasn't entirely aware that we are using Spotless for formatting; I knew that we use pre-commit hooks but didn't look into the tool behind it. I assumed the editorconfig was enforced - it turns out I was wrong.
2. I don't think this version of the editorconfig will collide with our JS Linters, as I only added stuff for Java and Python; the [*.java] or [*.py] headers inside the file indicate that.
3. IMHO We should not necessarily stick to custom linter rules; I see no benefit in that. We probably use Teamscale for a reason. If we don't want to ignore ca. 1700 bad practice findings, then either we need to revisit the custom rules or make every linting tool (spotless, coderabbit, teamscale, etc.) aware of those, right now actual findings go unnoticed because of the vast amount of false-positives.

What would you say if I revert all of the lines apart from the increased star import limit (as established in the last meeting, we want to eliminate the magic number of 4, my original intention with this PR) and thus hopefully make Teamscale helpful?

What do you think?

[Hialus]

1. I wasn't entirely aware that we are using Spotless for formatting; I knew that we use pre-commit hooks but didn't look into the tool behind it. I assumed the editorconfig was enforced - it turns out I was wrong.

I am also not fully aware of all the different linters and formatters we use, which is a big reason why I am against such massive changes. Ensuring that these changes are all coordinated is sadly everything but trivial.

2. I don't think this version of the editorconfig will collide with our JS Linters, as I only added stuff for Java and Python; the [*.java] or [*.py] headers inside the file indicate that.

I know. This was an example of another difference between IntelliJ's default and our custom rules. This would be a case that should be handled in the editorconfig file (instead of e.g. defining the suffix for auto generated subclasses)

3. IMHO We should not necessarily stick to custom linter rules; I see no benefit in that. We probably use Teamscale for a reason. If we don't want to ignore ca. 1700 bad practice findings, then either we need to revisit the custom rules or make every linting tool (spotless, coderabbit, teamscale, etc.) aware of those, right now actual findings go unnoticed because of the vast amount of false-positives.

We also can't just throw away all of our existing rules. In the last meeting we merely discussed changing the star import rule, not completely redoing our linter/formatter rules.
I'd be glad if we could discuss some of the other existing rules in detail with the other devs and Stephan, as e.g. the "else on a separate line" isn't really that great imo.
And again as I said adding/changing rules is a non-trivial thing, as it will need to be coordinated across different tools, so we should go it one step at a time.

What would you say if I revert all of the lines apart from the increased star import limit (as established in the last meeting, we want to eliminate the magic number of 4, my original intention with this PR) and thus hopefully make Teamscale helpful?

That would be fine, as that is what we already agreed upon. Though again I think it would be great if this could also be put into our spotless config (as well as other linters if possible). Ideally our own server code style GitHub run would fail if we add new star imports, so that we do not rely on Teamscale for this.

[reschandreas]

@Hialus, I removed everything except the changes to the usage of star imports. I also aligned the import order specified by Spotless. Now, if you optimize the imports using IntelliJ, it will expand them and remove the lines between the groups (didn't find anything for that). Spotless will validate the imports and readd the blank lines in between. I also couldn't find a setting for Spotless to replace star imports, but I guess with this setting, our code should produce fewer false positives.

[Strohgelaender]

I'm not sure if I understood the changes / goals right. My current understanding is the following:

The goal of this PR is to change our approach to star imports in order to get rid of teamscale and coderabbit warnings.
You accived this by changing the editorconfig threshold for start imports, but without touching the spotless configuration.

This means that if I change a file with star imports in one of my PRs, nothing will happen for now, and no tool will complain. The star imports will only get removed when using the "optimize imports" feature of IntelliJ.

At least in my workflow, the IntelliJ formatter does not play a role. If I want to reformat the code, I quickly execute spotless.

This means that in this form this change will create an inconsistent code style, depending on the current workflow of the developer. I'd like to avoid this.

Please check if you can enforce star import rules via spotless. Then violations will be automatically corrected in the pre-commit-hook and would lead to a failure in the server-style check. The editorconfig file in its current form is not meant to define its own rules, but instead to translate existing rules to the IDE which will then reduce the changes spotless needs to perform.

Regarding the PR size: If the configuration change is small (e.g. only one changed rule), I'm ok with bigger PRs (considering LOC) which enures a consistent code base. But I also agree with Timor, we should touch only 1-2 rules in such PRs to keep them reviewable.

---

My sugesstion would be to

• Keep the changes to editorconfig to avoid generating star imports when working on the codebase.
• Apply the IntelliJ "Optimize Imports" once in this PR for all files to remove star imports consistently.
• Add the following check to spotless to enforce no star imports in the future:

custom 'Refuse wildcard imports', {
  // Wildcard imports can't be resolved by spotless itself.
  // This will require the developer themselves to adhere to best practices.
  if (it =~ /\nimport .*\*;/) {
    throw new AssertionError("Do not use wildcard imports.  'spotlessApply' cannot resolve this issue.")
  }
}

This in combination with the editorconfig changes should not lead to failures when regularly working with the code, but will throw an error if a developer explicitly adds a star import, maintaining consistency across the code base.

References:

• Spotless Issue on that topic: Add remove wildcards imports when formating and ordering imports. diffplug/spotless#649
• Source of the custom check above: SOLR-16297: enable check on wildcards apache/solr#942
• An automatic transformation of star imports should be possible once spotless integrates eclipse code cleanup tasks, see issue Add support for Eclipse Clean Up diffplug/spotless#292.

[reschandreas]

Hi @Strohgelaender,

sorry I didn't reply earlier.

You're right. My goal/idea is to gradually phase out star imports as they keep causing false positives and leading to a complete neglect of real findings.

As I couldn't find a way to use Spotless only for changed files of a commit, I would rather merge these changes that prevent new ones and after some time, apply the config for all files that are left. This would decrease the potential merge conflicts and developer time for those changes.

I agree that this results in a temporarily inconsistent code styling, but after all, it's just imports

[Strohgelaender]

As discussied I'm ok with this approach. In a few months we should open another PR making the star import usage consistent and also implement the custom rule mentioned above.

[milljoniaer]

Nice, looks good 🚀

[rstief]

code lgtm