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

Change code style to refer to the one from maven-shared-resources #329

Merged
merged 4 commits into from
Oct 14, 2022

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Oct 12, 2022

PR following up apache/maven-shared-resources#1 to use a single reference for code style formatting.

@cstamas
Copy link
Member

cstamas commented Oct 12, 2022

Maybe point the download links to ASF gitbox instead of a "just a mirror" GH?

Comment on lines 148 to 150
Download <<<{{{https://github.com/apache/maven-shared-resources/blob/master/src/main/resources/config/eclipse-formatter-config.xml}eclipse-formatter-config.xml}}>>>
and import it into IDEA using Intellij IDEA > Preferences > Editor > Code Style > Java > Gear icon >
Import Scheme > Eclipse XML Profile
Copy link
Member

Choose a reason for hiding this comment

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

Link is for eclipse-formatter-config.xml

Copy link
Contributor Author

@gnodet gnodet Oct 12, 2022

Choose a reason for hiding this comment

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

@slawekjaranowski YES ! A single file is used. The recent versions of IDEA allow importing eclipse config files 😀

Copy link
Member

Choose a reason for hiding this comment

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

I don't see such options:
image

IntelliJ IDEA 2022.2.2 (Ultimate Edition)

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

But this is NOT a "latest" IDEA feature at all, it is since a while, that IDEA supports eclipse XML import....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as @cstamas on 2022.2.1 Community Edition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@slawekjaranowski Eclipse Interoperability plugin disabled maybe? (by default is enabled, unless you tampered with it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@gnodet
Copy link
Contributor Author

gnodet commented Oct 12, 2022

Btw, given we enforce a 120 characters hard break on lines, I wonder if we should switch to header formatted in the same way, something like

/*
 * Licensed to the Apache Software Foundation (ASF) under one or more contributor license 
 * agreements. See the NOTICE file distributed with this work for additional information 
 * regarding copyright ownership. The ASF licenses this file to you under the Apache License, 
 * Version 2.0 (the "License"); you may not use this file except in compliance with the 
 * License. You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software distributed under the 
 * License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, 
 * either express or implied. See the License for the specific language governing permissions 
 * and limitations under the License.
 */

@slawekjaranowski
Copy link
Member

@cstamas Eclipse Interoperability helps 😄

@slawekjaranowski
Copy link
Member

I tested importing importing eclipse formats ... into InteliJ,
We have missing configurations for:

  • imports order - settings -> Code Style -> Java - imports tab has default values

how we will manage it?

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

We should mention that plugin Eclipse Interoperability must be enable.

@gnodet
Copy link
Contributor Author

gnodet commented Oct 12, 2022

I tested importing importing eclipse formats ... into InteliJ, We have missing configurations for:

  • imports order - settings -> Code Style -> Java - imports tab has default values

how we will manage it?

Meh, you're right.

We could tell users that imports won't be sorted and that they need to either add those or rely on the impsort plugin wich will fix those.

Alternatively, we can add back idea settings, but I'd rather avoid it. We can generate it by importing the eclipse settings into IDEA, fix the import settings to comply with maven code style and export those settings. But we'd have to put them in maven-shared-resources also I suppose.

@slawekjaranowski
Copy link
Member

I tested importing importing eclipse formats ... into InteliJ, We have missing configurations for:

  • imports order - settings -> Code Style -> Java - imports tab has default values

how we will manage it?

Meh, you're right.

We could tell users that imports won't be sorted and that they need to either add those or rely on the impsort plugin wich will fix those.

Alternatively, we can add back idea settings, but I'd rather avoid it. We can generate it by importing the eclipse settings into IDEA, fix the import settings to comply with maven code style and export those settings. But we'd have to put them in maven-shared-resources also I suppose.

I'm for preserve native settings for InteliJ, it can be in m-shared-r.
I hope that not will be changed so frequently, so we can do such manual works as you describe, to have synchronized Eclipse and InteliJ.

It will be easier for user to simply import settings.

We have also import settings for eclipse in https://github.com/apache/maven-site/blob/master/content/resources/developers/maven-eclipse.importorder so this file should be moved to shared to have everything in one place.

@gnodet gnodet merged commit 52ad385 into apache:master Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants