[infinispan-dev] Style errors :(

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[infinispan-dev] Style errors :(

Galder Zamarreño
Hi all,

After integrating [1] I'm getting build errors such as:

[INFO] --- maven-checkstyle-plugin:2.17:checkstyle (checkstyle) @ infinispan-core ---
[INFO] Starting audit...
/home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/internal/InternalExternalizerTable.java:55: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
/home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/ExternalizerTable.java:76: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
/home/g/0/infinispan/git/core/src/test/java/org/infinispan/filter/CompositeKeyValueFilterConverter.java:11:1: error: Duplicate import to line 10 - org.infinispan.metadata.Metadata.

Even after installing the latest style for IntelliJ [2], reformatting InternalExternalizerTable.java won't fix those how errors.

So, what do we do? :(

Cheers,

[1] https://github.com/infinispan/infinispan/commit/313b19301055c6267c6f2ea9065a7ab1b68099fe
[2] https://github.com/infinispan/infinispan/blob/master/ide-settings/intellij/IntelliJ_IDEA_Code_Style.jar
--
Galder Zamarreño
Infinispan, Red Hat


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Style errors :(

Sanne Grinovero-3
Please don't forget Eclipse users. It's hard enough to contribute to
Infinispan for non-IDEA users; if you enforce specific style rules, at
the very least provide compatible formatters for the other IDEs or the
barrier for contributors becomes too high.

Galder: to answer your specific question I'd personally say you should
revert the offending commit. I don't know who did it nor who merged
it, but it's just not nice for others that such things get merged
carelessly. Errors can be forgiven, but should be fixed by those who
create the mess ;)

On 16 August 2016 at 16:37, Galder Zamarreño <[hidden email]> wrote:

> Hi all,
>
> After integrating [1] I'm getting build errors such as:
>
> [INFO] --- maven-checkstyle-plugin:2.17:checkstyle (checkstyle) @ infinispan-core ---
> [INFO] Starting audit...
> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/internal/InternalExternalizerTable.java:55: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/ExternalizerTable.java:76: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
> /home/g/0/infinispan/git/core/src/test/java/org/infinispan/filter/CompositeKeyValueFilterConverter.java:11:1: error: Duplicate import to line 10 - org.infinispan.metadata.Metadata.
>
> Even after installing the latest style for IntelliJ [2], reformatting InternalExternalizerTable.java won't fix those how errors.
>
> So, what do we do? :(
>
> Cheers,
>
> [1] https://github.com/infinispan/infinispan/commit/313b19301055c6267c6f2ea9065a7ab1b68099fe
> [2] https://github.com/infinispan/infinispan/blob/master/ide-settings/intellij/IntelliJ_IDEA_Code_Style.jar
> --
> Galder Zamarreño
> Infinispan, Red Hat
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Style errors :(

Sebastian Laskawiec
Hey Galder, Sanne!

The author is no longer anonymous! It was me [1]!

Jokes aside; Galder - may I ask you to double check your configuration? I extracted formatter archive and it seems to be fine [2]. Perhaps you didn't enable it (watch out, the formatter name was changed some time ago from Horizon to Infinispan) or this is some tooling problem (which probably should be reported to Jetbrains). Anyway - could you please double check it? I'll try to catch you tomorrow on IRC and talk about it...

BTW - There are 2 different errors in your output - the first one is the start import which should be avoided. The second one is duplicated import (also should be avoided). Please don't confuse those two.

@Sanne - before you trow a 'git revert' axe on me - may I ask you to import the latest formatter and check if the amount of imports before replacing them with star looks correctly? It looks fine to me but I may have some old installation, old configuration etc...

Thanks
Sebastian "the Troublemaker"

[2] unzip IntelliJ_IDEA_Code_Style.jar and look into attached XML. Those two properties are responsible for star imports. 
<option name="CLASS_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />
<option name="NAMES_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />


On Tue, Aug 16, 2016 at 8:21 PM, Sanne Grinovero <[hidden email]> wrote:
Please don't forget Eclipse users. It's hard enough to contribute to
Infinispan for non-IDEA users; if you enforce specific style rules, at
the very least provide compatible formatters for the other IDEs or the
barrier for contributors becomes too high.

Galder: to answer your specific question I'd personally say you should
revert the offending commit. I don't know who did it nor who merged
it, but it's just not nice for others that such things get merged
carelessly. Errors can be forgiven, but should be fixed by those who
create the mess ;)

On 16 August 2016 at 16:37, Galder Zamarreño <[hidden email]> wrote:
> Hi all,
>
> After integrating [1] I'm getting build errors such as:
>
> [INFO] --- maven-checkstyle-plugin:2.17:checkstyle (checkstyle) @ infinispan-core ---
> [INFO] Starting audit...
> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/internal/InternalExternalizerTable.java:55: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/ExternalizerTable.java:76: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
> /home/g/0/infinispan/git/core/src/test/java/org/infinispan/filter/CompositeKeyValueFilterConverter.java:11:1: error: Duplicate import to line 10 - org.infinispan.metadata.Metadata.
>
> Even after installing the latest style for IntelliJ [2], reformatting InternalExternalizerTable.java won't fix those how errors.
>
> So, what do we do? :(
>
> Cheers,
>
> [1] https://github.com/infinispan/infinispan/commit/313b19301055c6267c6f2ea9065a7ab1b68099fe
> [2] https://github.com/infinispan/infinispan/blob/master/ide-settings/intellij/IntelliJ_IDEA_Code_Style.jar
> --
> Galder Zamarreño
> Infinispan, Red Hat
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Style errors :(

Sebastian Laskawiec
I also integrated Adrian's PR with additional fixed (thank you Adrian!): https://github.com/infinispan/infinispan/pull/4510

The master branch builds fine now.

Thanks
Sebastian

On Tue, Aug 16, 2016 at 10:16 PM, Sebastian Laskawiec <[hidden email]> wrote:
Hey Galder, Sanne!

The author is no longer anonymous! It was me [1]!

Jokes aside; Galder - may I ask you to double check your configuration? I extracted formatter archive and it seems to be fine [2]. Perhaps you didn't enable it (watch out, the formatter name was changed some time ago from Horizon to Infinispan) or this is some tooling problem (which probably should be reported to Jetbrains). Anyway - could you please double check it? I'll try to catch you tomorrow on IRC and talk about it...

BTW - There are 2 different errors in your output - the first one is the start import which should be avoided. The second one is duplicated import (also should be avoided). Please don't confuse those two.

@Sanne - before you trow a 'git revert' axe on me - may I ask you to import the latest formatter and check if the amount of imports before replacing them with star looks correctly? It looks fine to me but I may have some old installation, old configuration etc...

Thanks
Sebastian "the Troublemaker"

[2] unzip IntelliJ_IDEA_Code_Style.jar and look into attached XML. Those two properties are responsible for star imports. 
<option name="CLASS_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />
<option name="NAMES_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />


On Tue, Aug 16, 2016 at 8:21 PM, Sanne Grinovero <[hidden email]> wrote:
Please don't forget Eclipse users. It's hard enough to contribute to
Infinispan for non-IDEA users; if you enforce specific style rules, at
the very least provide compatible formatters for the other IDEs or the
barrier for contributors becomes too high.

Galder: to answer your specific question I'd personally say you should
revert the offending commit. I don't know who did it nor who merged
it, but it's just not nice for others that such things get merged
carelessly. Errors can be forgiven, but should be fixed by those who
create the mess ;)

On 16 August 2016 at 16:37, Galder Zamarreño <[hidden email]> wrote:
> Hi all,
>
> After integrating [1] I'm getting build errors such as:
>
> [INFO] --- maven-checkstyle-plugin:2.17:checkstyle (checkstyle) @ infinispan-core ---
> [INFO] Starting audit...
> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/internal/InternalExternalizerTable.java:55: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/ExternalizerTable.java:76: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
> /home/g/0/infinispan/git/core/src/test/java/org/infinispan/filter/CompositeKeyValueFilterConverter.java:11:1: error: Duplicate import to line 10 - org.infinispan.metadata.Metadata.
>
> Even after installing the latest style for IntelliJ [2], reformatting InternalExternalizerTable.java won't fix those how errors.
>
> So, what do we do? :(
>
> Cheers,
>
> [1] https://github.com/infinispan/infinispan/commit/313b19301055c6267c6f2ea9065a7ab1b68099fe
> [2] https://github.com/infinispan/infinispan/blob/master/ide-settings/intellij/IntelliJ_IDEA_Code_Style.jar
> --
> Galder Zamarreño
> Infinispan, Red Hat
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev



_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Style errors :(

Adrian Nistor
The master branch was building fine anyway because checkstyle does not seem to be included in default lifecycle yet, so I'm not sure why this generates so much commotion.
But we should stop being lenient about these checkstyle violations and fail the build from now on. ATM there are no more violations for the currently enabled rules, so it is about time :).
This approach seems to work fine for hibernate-ogm and hibernate-search, and they have tons of style rules. We only have three.

On 08/16/2016 11:25 PM, Sebastian Laskawiec wrote:
I also integrated Adrian's PR with additional fixed (thank you Adrian!): https://github.com/infinispan/infinispan/pull/4510

The master branch builds fine now.

Thanks
Sebastian

On Tue, Aug 16, 2016 at 10:16 PM, Sebastian Laskawiec <[hidden email]> wrote:
Hey Galder, Sanne!

The author is no longer anonymous! It was me [1]!

Jokes aside; Galder - may I ask you to double check your configuration? I extracted formatter archive and it seems to be fine [2]. Perhaps you didn't enable it (watch out, the formatter name was changed some time ago from Horizon to Infinispan) or this is some tooling problem (which probably should be reported to Jetbrains). Anyway - could you please double check it? I'll try to catch you tomorrow on IRC and talk about it...

BTW - There are 2 different errors in your output - the first one is the start import which should be avoided. The second one is duplicated import (also should be avoided). Please don't confuse those two.

@Sanne - before you trow a 'git revert' axe on me - may I ask you to import the latest formatter and check if the amount of imports before replacing them with star looks correctly? It looks fine to me but I may have some old installation, old configuration etc...

Thanks
Sebastian "the Troublemaker"

[2] unzip IntelliJ_IDEA_Code_Style.jar and look into attached XML. Those two properties are responsible for star imports. 
<option name="CLASS_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />
<option name="NAMES_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />


On Tue, Aug 16, 2016 at 8:21 PM, Sanne Grinovero <[hidden email]> wrote:
Please don't forget Eclipse users. It's hard enough to contribute to
Infinispan for non-IDEA users; if you enforce specific style rules, at
the very least provide compatible formatters for the other IDEs or the
barrier for contributors becomes too high.

Galder: to answer your specific question I'd personally say you should
revert the offending commit. I don't know who did it nor who merged
it, but it's just not nice for others that such things get merged
carelessly. Errors can be forgiven, but should be fixed by those who
create the mess ;)

On 16 August 2016 at 16:37, Galder Zamarreño <[hidden email]> wrote:
> Hi all,
>
> After integrating [1] I'm getting build errors such as:
>
> [INFO] --- maven-checkstyle-plugin:2.17:checkstyle (checkstyle) @ infinispan-core ---
> [INFO] Starting audit...
> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/internal/InternalExternalizerTable.java:55: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/ExternalizerTable.java:76: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
> /home/g/0/infinispan/git/core/src/test/java/org/infinispan/filter/CompositeKeyValueFilterConverter.java:11:1: error: Duplicate import to line 10 - org.infinispan.metadata.Metadata.
>
> Even after installing the latest style for IntelliJ [2], reformatting InternalExternalizerTable.java won't fix those how errors.
>
> So, what do we do? :(
>
> Cheers,
>
> [1] https://github.com/infinispan/infinispan/commit/313b19301055c6267c6f2ea9065a7ab1b68099fe
> [2] https://github.com/infinispan/infinispan/blob/master/ide-settings/intellij/IntelliJ_IDEA_Code_Style.jar
> --
> Galder Zamarreño
> Infinispan, Red Hat
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev




_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev



_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Style errors :(

Sebastian Laskawiec
Hey Adrian!

+1. There are some more rules in our checkstyle module and I'm planning to turn them on soon (however I agree, the import one introduces probably the biggest amount of changes).

Checkstyle should be enabled by default [3] and it executes fine on my local machine. However if you're not building the checkstyle module with the same reactor as the rest of the project, it may happen that Maven downloads it from Nexus (this is exactly what happened in case of [4]). The situation should be fixed automatically as soon as TeamCity rebuilds and redeploys new SNAPSHOT. 

Thanks
Sebastian


On Wed, Aug 17, 2016 at 1:01 AM, Adrian Nistor <[hidden email]> wrote:
The master branch was building fine anyway because checkstyle does not seem to be included in default lifecycle yet, so I'm not sure why this generates so much commotion.
But we should stop being lenient about these checkstyle violations and fail the build from now on. ATM there are no more violations for the currently enabled rules, so it is about time :).
This approach seems to work fine for hibernate-ogm and hibernate-search, and they have tons of style rules. We only have three.


On 08/16/2016 11:25 PM, Sebastian Laskawiec wrote:
I also integrated Adrian's PR with additional fixed (thank you Adrian!): https://github.com/infinispan/infinispan/pull/4510

The master branch builds fine now.

Thanks
Sebastian

On Tue, Aug 16, 2016 at 10:16 PM, Sebastian Laskawiec <[hidden email]> wrote:
Hey Galder, Sanne!

The author is no longer anonymous! It was me [1]!

Jokes aside; Galder - may I ask you to double check your configuration? I extracted formatter archive and it seems to be fine [2]. Perhaps you didn't enable it (watch out, the formatter name was changed some time ago from Horizon to Infinispan) or this is some tooling problem (which probably should be reported to Jetbrains). Anyway - could you please double check it? I'll try to catch you tomorrow on IRC and talk about it...

BTW - There are 2 different errors in your output - the first one is the start import which should be avoided. The second one is duplicated import (also should be avoided). Please don't confuse those two.

@Sanne - before you trow a 'git revert' axe on me - may I ask you to import the latest formatter and check if the amount of imports before replacing them with star looks correctly? It looks fine to me but I may have some old installation, old configuration etc...

Thanks
Sebastian "the Troublemaker"

[2] unzip IntelliJ_IDEA_Code_Style.jar and look into attached XML. Those two properties are responsible for star imports. 
<option name="CLASS_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />
<option name="NAMES_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />


On Tue, Aug 16, 2016 at 8:21 PM, Sanne Grinovero <[hidden email]> wrote:
Please don't forget Eclipse users. It's hard enough to contribute to
Infinispan for non-IDEA users; if you enforce specific style rules, at
the very least provide compatible formatters for the other IDEs or the
barrier for contributors becomes too high.

Galder: to answer your specific question I'd personally say you should
revert the offending commit. I don't know who did it nor who merged
it, but it's just not nice for others that such things get merged
carelessly. Errors can be forgiven, but should be fixed by those who
create the mess ;)

On 16 August 2016 at 16:37, Galder Zamarreño <[hidden email]> wrote:
> Hi all,
>
> After integrating [1] I'm getting build errors such as:
>
> [INFO] --- maven-checkstyle-plugin:2.17:checkstyle (checkstyle) @ infinispan-core ---
> [INFO] Starting audit...
> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/internal/InternalExternalizerTable.java:55: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/ExternalizerTable.java:76: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
> /home/g/0/infinispan/git/core/src/test/java/org/infinispan/filter/CompositeKeyValueFilterConverter.java:11:1: error: Duplicate import to line 10 - org.infinispan.metadata.Metadata.
>
> Even after installing the latest style for IntelliJ [2], reformatting InternalExternalizerTable.java won't fix those how errors.
>
> So, what do we do? :(
>
> Cheers,
>
> [1] https://github.com/infinispan/infinispan/commit/313b19301055c6267c6f2ea9065a7ab1b68099fe
> [2] https://github.com/infinispan/infinispan/blob/master/ide-settings/intellij/IntelliJ_IDEA_Code_Style.jar
> --
> Galder Zamarreño
> Infinispan, Red Hat
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev




_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev



_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Style errors :(

Galder Zamarreño
In reply to this post by Adrian Nistor

--
Galder Zamarreño
Infinispan, Red Hat

> On 17 Aug 2016, at 01:01, Adrian Nistor <[hidden email]> wrote:
>
> The master branch was building fine anyway because checkstyle does not seem to be included in default lifecycle yet, so I'm not sure why this generates so much commotion.

Hmmm, when I execute "mvn -DskipTests=true clean install --projects core -am", it appears as part of the default lifecycle:

Here: https://gist.github.com/galderz/16b126afaf6adb801473ac58c1d5e504#file-gistfile1-txt-L96
Here: https://gist.github.com/galderz/16b126afaf6adb801473ac58c1d5e504#file-gistfile1-txt-L156
And here: https://gist.github.com/galderz/16b126afaf6adb801473ac58c1d5e504#file-gistfile1-txt-L356

> But we should stop being lenient about these checkstyle violations and fail the build from now on. ATM there are no more violations for the currently enabled rules, so it is about time :).

I'm happy for that, as long as the IDEs can quickly fix those. I'm not manually handrolling things to make checkstyle happy :|

> This approach seems to work fine for hibernate-ogm and hibernate-search, and they have tons of style rules. We only have three.
>
> On 08/16/2016 11:25 PM, Sebastian Laskawiec wrote:
>> I also integrated Adrian's PR with additional fixed (thank you Adrian!): https://github.com/infinispan/infinispan/pull/4510
>>
>> The master branch builds fine now.
>>
>> Thanks
>> Sebastian
>>
>> On Tue, Aug 16, 2016 at 10:16 PM, Sebastian Laskawiec <[hidden email]> wrote:
>> Hey Galder, Sanne!
>>
>> The author is no longer anonymous! It was me [1]!
>>
>> Jokes aside; Galder - may I ask you to double check your configuration? I extracted formatter archive and it seems to be fine [2]. Perhaps you didn't enable it (watch out, the formatter name was changed some time ago from Horizon to Infinispan) or this is some tooling problem (which probably should be reported to Jetbrains). Anyway - could you please double check it? I'll try to catch you tomorrow on IRC and talk about it...
>>
>> BTW - There are 2 different errors in your output - the first one is the start import which should be avoided. The second one is duplicated import (also should be avoided). Please don't confuse those two.
>>
>> @Sanne - before you trow a 'git revert' axe on me - may I ask you to import the latest formatter and check if the amount of imports before replacing them with star looks correctly? It looks fine to me but I may have some old installation, old configuration etc...
>>
>> Thanks
>> Sebastian "the Troublemaker"
>>
>> [1] https://www.youtube.com/watch?v=a43kowi2ncI
>> [2] unzip IntelliJ_IDEA_Code_Style.jar and look into attached XML. Those two properties are responsible for star imports.
>> <option name="CLASS_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />
>> <option name="NAMES_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />
>>
>>
>> On Tue, Aug 16, 2016 at 8:21 PM, Sanne Grinovero <[hidden email]> wrote:
>> Please don't forget Eclipse users. It's hard enough to contribute to
>> Infinispan for non-IDEA users; if you enforce specific style rules, at
>> the very least provide compatible formatters for the other IDEs or the
>> barrier for contributors becomes too high.
>>
>> Galder: to answer your specific question I'd personally say you should
>> revert the offending commit. I don't know who did it nor who merged
>> it, but it's just not nice for others that such things get merged
>> carelessly. Errors can be forgiven, but should be fixed by those who
>> create the mess ;)
>>
>> On 16 August 2016 at 16:37, Galder Zamarreño <[hidden email]> wrote:
>> > Hi all,
>> >
>> > After integrating [1] I'm getting build errors such as:
>> >
>> > [INFO] --- maven-checkstyle-plugin:2.17:checkstyle (checkstyle) @ infinispan-core ---
>> > [INFO] Starting audit...
>> > /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/internal/InternalExternalizerTable.java:55: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
>> > /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/ExternalizerTable.java:76: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
>> > /home/g/0/infinispan/git/core/src/test/java/org/infinispan/filter/CompositeKeyValueFilterConverter.java:11:1: error: Duplicate import to line 10 - org.infinispan.metadata.Metadata.
>> >
>> > Even after installing the latest style for IntelliJ [2], reformatting InternalExternalizerTable.java won't fix those how errors.
>> >
>> > So, what do we do? :(
>> >
>> > Cheers,
>> >
>> > [1] https://github.com/infinispan/infinispan/commit/313b19301055c6267c6f2ea9065a7ab1b68099fe
>> > [2] https://github.com/infinispan/infinispan/blob/master/ide-settings/intellij/IntelliJ_IDEA_Code_Style.jar
>> > --
>> > Galder Zamarreño
>> > Infinispan, Red Hat
>> >
>> >
>> > _______________________________________________
>> > infinispan-dev mailing list
>> > [hidden email]
>> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>>
>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>>
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Style errors :(

Galder Zamarreño
In reply to this post by Sebastian Laskawiec
I've double checked the configuration and it looks as you mention. The IDE dialog also shows the values you indicate but reformatting still does not seem to unroll the * import :|

Cheers,
--
Galder Zamarreño
Infinispan, Red Hat

> On 16 Aug 2016, at 22:16, Sebastian Laskawiec <[hidden email]> wrote:
>
> Hey Galder, Sanne!
>
> The author is no longer anonymous! It was me [1]!
>
> Jokes aside; Galder - may I ask you to double check your configuration? I extracted formatter archive and it seems to be fine [2]. Perhaps you didn't enable it (watch out, the formatter name was changed some time ago from Horizon to Infinispan) or this is some tooling problem (which probably should be reported to Jetbrains). Anyway - could you please double check it? I'll try to catch you tomorrow on IRC and talk about it...
>
> BTW - There are 2 different errors in your output - the first one is the start import which should be avoided. The second one is duplicated import (also should be avoided). Please don't confuse those two.
>
> @Sanne - before you trow a 'git revert' axe on me - may I ask you to import the latest formatter and check if the amount of imports before replacing them with star looks correctly? It looks fine to me but I may have some old installation, old configuration etc...
>
> Thanks
> Sebastian "the Troublemaker"
>
> [1] https://www.youtube.com/watch?v=a43kowi2ncI
> [2] unzip IntelliJ_IDEA_Code_Style.jar and look into attached XML. Those two properties are responsible for star imports.
> <option name="CLASS_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />
> <option name="NAMES_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />
>
>
> On Tue, Aug 16, 2016 at 8:21 PM, Sanne Grinovero <[hidden email]> wrote:
> Please don't forget Eclipse users. It's hard enough to contribute to
> Infinispan for non-IDEA users; if you enforce specific style rules, at
> the very least provide compatible formatters for the other IDEs or the
> barrier for contributors becomes too high.
>
> Galder: to answer your specific question I'd personally say you should
> revert the offending commit. I don't know who did it nor who merged
> it, but it's just not nice for others that such things get merged
> carelessly. Errors can be forgiven, but should be fixed by those who
> create the mess ;)
>
> On 16 August 2016 at 16:37, Galder Zamarreño <[hidden email]> wrote:
> > Hi all,
> >
> > After integrating [1] I'm getting build errors such as:
> >
> > [INFO] --- maven-checkstyle-plugin:2.17:checkstyle (checkstyle) @ infinispan-core ---
> > [INFO] Starting audit...
> > /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/internal/InternalExternalizerTable.java:55: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
> > /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/ExternalizerTable.java:76: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
> > /home/g/0/infinispan/git/core/src/test/java/org/infinispan/filter/CompositeKeyValueFilterConverter.java:11:1: error: Duplicate import to line 10 - org.infinispan.metadata.Metadata.
> >
> > Even after installing the latest style for IntelliJ [2], reformatting InternalExternalizerTable.java won't fix those how errors.
> >
> > So, what do we do? :(
> >
> > Cheers,
> >
> > [1] https://github.com/infinispan/infinispan/commit/313b19301055c6267c6f2ea9065a7ab1b68099fe
> > [2] https://github.com/infinispan/infinispan/blob/master/ide-settings/intellij/IntelliJ_IDEA_Code_Style.jar
> > --
> > Galder Zamarreño
> > Infinispan, Red Hat
> >
> >
> > _______________________________________________
> > infinispan-dev mailing list
> > [hidden email]
> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Style errors :(

Radim Vansa
Galder, in Editor/General/Auto import there's Optimize imports on the
fly. Without that, IDEA won't mess with imports, so make sure it's
checked if you want to enforce the imports order.

Anyway, I personally think that order of imports (which are hidden by
IDEs anyway) is not important at all, at it shouldn't be enforced by
checkstyle. The only trouble is that people have the switch above on
(it's default in IDEA) - or similiar option in other IDEs - and then IDE
changes the order and generates fuss in the PR.

Regrettably IDEA does not have per-module coding style setting [1], so
if you have multiple projects (which use different coding style) in
single workspace, it gets into the way.

Radim

[1] https://youtrack.jetbrains.com/issue/IDEA-69685

On 08/17/2016 08:40 AM, Galder Zamarreño wrote:

> I've double checked the configuration and it looks as you mention. The IDE dialog also shows the values you indicate but reformatting still does not seem to unroll the * import :|
>
> Cheers,
> --
> Galder Zamarreño
> Infinispan, Red Hat
>
>> On 16 Aug 2016, at 22:16, Sebastian Laskawiec <[hidden email]> wrote:
>>
>> Hey Galder, Sanne!
>>
>> The author is no longer anonymous! It was me [1]!
>>
>> Jokes aside; Galder - may I ask you to double check your configuration? I extracted formatter archive and it seems to be fine [2]. Perhaps you didn't enable it (watch out, the formatter name was changed some time ago from Horizon to Infinispan) or this is some tooling problem (which probably should be reported to Jetbrains). Anyway - could you please double check it? I'll try to catch you tomorrow on IRC and talk about it...
>>
>> BTW - There are 2 different errors in your output - the first one is the start import which should be avoided. The second one is duplicated import (also should be avoided). Please don't confuse those two.
>>
>> @Sanne - before you trow a 'git revert' axe on me - may I ask you to import the latest formatter and check if the amount of imports before replacing them with star looks correctly? It looks fine to me but I may have some old installation, old configuration etc...
>>
>> Thanks
>> Sebastian "the Troublemaker"
>>
>> [1] https://www.youtube.com/watch?v=a43kowi2ncI
>> [2] unzip IntelliJ_IDEA_Code_Style.jar and look into attached XML. Those two properties are responsible for star imports.
>> <option name="CLASS_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />
>> <option name="NAMES_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />
>>
>>
>> On Tue, Aug 16, 2016 at 8:21 PM, Sanne Grinovero <[hidden email]> wrote:
>> Please don't forget Eclipse users. It's hard enough to contribute to
>> Infinispan for non-IDEA users; if you enforce specific style rules, at
>> the very least provide compatible formatters for the other IDEs or the
>> barrier for contributors becomes too high.
>>
>> Galder: to answer your specific question I'd personally say you should
>> revert the offending commit. I don't know who did it nor who merged
>> it, but it's just not nice for others that such things get merged
>> carelessly. Errors can be forgiven, but should be fixed by those who
>> create the mess ;)
>>
>> On 16 August 2016 at 16:37, Galder Zamarreño <[hidden email]> wrote:
>>> Hi all,
>>>
>>> After integrating [1] I'm getting build errors such as:
>>>
>>> [INFO] --- maven-checkstyle-plugin:2.17:checkstyle (checkstyle) @ infinispan-core ---
>>> [INFO] Starting audit...
>>> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/internal/InternalExternalizerTable.java:55: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
>>> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/ExternalizerTable.java:76: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
>>> /home/g/0/infinispan/git/core/src/test/java/org/infinispan/filter/CompositeKeyValueFilterConverter.java:11:1: error: Duplicate import to line 10 - org.infinispan.metadata.Metadata.
>>>
>>> Even after installing the latest style for IntelliJ [2], reformatting InternalExternalizerTable.java won't fix those how errors.
>>>
>>> So, what do we do? :(
>>>
>>> Cheers,
>>>
>>> [1] https://github.com/infinispan/infinispan/commit/313b19301055c6267c6f2ea9065a7ab1b68099fe
>>> [2] https://github.com/infinispan/infinispan/blob/master/ide-settings/intellij/IntelliJ_IDEA_Code_Style.jar
>>> --
>>> Galder Zamarreño
>>> Infinispan, Red Hat
>>>
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] Style errors :(

Sanne Grinovero-3
On 17 August 2016 at 08:24, Radim Vansa <[hidden email]> wrote:
> Galder, in Editor/General/Auto import there's Optimize imports on the
> fly. Without that, IDEA won't mess with imports, so make sure it's
> checked if you want to enforce the imports order.
>
> Anyway, I personally think that order of imports (which are hidden by
> IDEs anyway) is not important at all, at it shouldn't be enforced by
> checkstyle. The only trouble is that people have the switch above on
> (it's default in IDEA) - or similiar option in other IDEs - and then IDE
> changes the order and generates fuss in the PR.

+1 better tread carefully, I predict a lot of pain here.

And since you all mentioned Hibernate Search as an example of were it
works, let me share some hints:
some rules are important as for example importing "*" has consequences
on determinism, so there's a small subset of rules which I'd highly
recommend.
Another example is that we actively use checsstyle rules to ban usage
of types which should never be used, e.g. mistakenly use the JGroups
assertions instead of a JUnit assertion:
https://github.com/hibernate/hibernate-search/blob/master/build-config/src/main/resources/checkstyle.xml#L173-L175

Then we also enforce our "style", but be aware that these are
controversial and often seen as a PITA by contributors so I'd feel the
need to stress that the important value is not really about
maintaining our weird style but rather about making sure that changes
in PR "stick to the point" and don't change anything else, this is
important to ease porting and rebasing of patches across branches.
In short, it's designed to _prevent_ people from sending PRs with
massive, automated or not, source code reformatting.

In conclusion, messing with import order seems a lot of pain and will
make your future rebases and backports a huge pain. I'd advise against
changes to the status quo but rather enforce such rules on
new/rewritten modules, or other cases in which you already know that
backports are not going to be likely.

Finally, be aware that some style rules can't possibly be enforced in
the same way across IDEs. Import order is one of the tricky ones so
unless you provide rules which you've tested and working you're being
very annoying to contributors.

I'm not against introducing it at all, I did the initial contribution
;) But be careful on what you enable, on which module, and when, and
ultimately what's the effect you want to achieve.

>
> Regrettably IDEA does not have per-module coding style setting [1], so
> if you have multiple projects (which use different coding style) in
> single workspace, it gets into the way.
>
> Radim
>
> [1] https://youtrack.jetbrains.com/issue/IDEA-69685
>
> On 08/17/2016 08:40 AM, Galder Zamarreño wrote:
>> I've double checked the configuration and it looks as you mention. The IDE dialog also shows the values you indicate but reformatting still does not seem to unroll the * import :|
>>
>> Cheers,
>> --
>> Galder Zamarreño
>> Infinispan, Red Hat
>>
>>> On 16 Aug 2016, at 22:16, Sebastian Laskawiec <[hidden email]> wrote:
>>>
>>> Hey Galder, Sanne!
>>>
>>> The author is no longer anonymous! It was me [1]!
>>>
>>> Jokes aside; Galder - may I ask you to double check your configuration? I extracted formatter archive and it seems to be fine [2]. Perhaps you didn't enable it (watch out, the formatter name was changed some time ago from Horizon to Infinispan) or this is some tooling problem (which probably should be reported to Jetbrains). Anyway - could you please double check it? I'll try to catch you tomorrow on IRC and talk about it...
>>>
>>> BTW - There are 2 different errors in your output - the first one is the start import which should be avoided. The second one is duplicated import (also should be avoided). Please don't confuse those two.
>>>
>>> @Sanne - before you trow a 'git revert' axe on me - may I ask you to import the latest formatter and check if the amount of imports before replacing them with star looks correctly? It looks fine to me but I may have some old installation, old configuration etc...
>>>
>>> Thanks
>>> Sebastian "the Troublemaker"
>>>
>>> [1] https://www.youtube.com/watch?v=a43kowi2ncI
>>> [2] unzip IntelliJ_IDEA_Code_Style.jar and look into attached XML. Those two properties are responsible for star imports.
>>> <option name="CLASS_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />
>>> <option name="NAMES_COUNT_TO_USE_IMPORT_ON_DEMAND" value="999" />
>>>
>>>
>>> On Tue, Aug 16, 2016 at 8:21 PM, Sanne Grinovero <[hidden email]> wrote:
>>> Please don't forget Eclipse users. It's hard enough to contribute to
>>> Infinispan for non-IDEA users; if you enforce specific style rules, at
>>> the very least provide compatible formatters for the other IDEs or the
>>> barrier for contributors becomes too high.
>>>
>>> Galder: to answer your specific question I'd personally say you should
>>> revert the offending commit. I don't know who did it nor who merged
>>> it, but it's just not nice for others that such things get merged
>>> carelessly. Errors can be forgiven, but should be fixed by those who
>>> create the mess ;)
>>>
>>> On 16 August 2016 at 16:37, Galder Zamarreño <[hidden email]> wrote:
>>>> Hi all,
>>>>
>>>> After integrating [1] I'm getting build errors such as:
>>>>
>>>> [INFO] --- maven-checkstyle-plugin:2.17:checkstyle (checkstyle) @ infinispan-core ---
>>>> [INFO] Starting audit...
>>>> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/internal/InternalExternalizerTable.java:55: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
>>>> /home/g/0/infinispan/git/core/src/main/java/org/infinispan/marshall/core/ExternalizerTable.java:76: error: Using the '.*' form of import should be avoided - org.infinispan.marshall.exts.*.
>>>> /home/g/0/infinispan/git/core/src/test/java/org/infinispan/filter/CompositeKeyValueFilterConverter.java:11:1: error: Duplicate import to line 10 - org.infinispan.metadata.Metadata.
>>>>
>>>> Even after installing the latest style for IntelliJ [2], reformatting InternalExternalizerTable.java won't fix those how errors.
>>>>
>>>> So, what do we do? :(
>>>>
>>>> Cheers,
>>>>
>>>> [1] https://github.com/infinispan/infinispan/commit/313b19301055c6267c6f2ea9065a7ab1b68099fe
>>>> [2] https://github.com/infinispan/infinispan/blob/master/ide-settings/intellij/IntelliJ_IDEA_Code_Style.jar
>>>> --
>>>> Galder Zamarreño
>>>> Infinispan, Red Hat
>>>>
>>>>
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> [hidden email]
>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
> --
> Radim Vansa <[hidden email]>
> JBoss Performance Team
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev