replay-framework / replay

Fork of the Play1 Framework
Other
20 stars 11 forks source link

Remove unused imports and run formatter #440

Closed cies closed 1 month ago

cies commented 1 month ago

Title says it all...

This is probably a first attempt, I'm personally against wildcard imports except for clearly defined cases (that RePlay has non of that I know), so I'd like to make them forbidden and run the cleanup again so IntelliJ can replace them.

Also RePlay splits up the imports in java(x) and the rest. I dont like this either, so I gladly also make this a rule and run the cleanup again to "fix" it.

These two points would be implemented by adding an ".editorconfig" to the root of the project (work like a charm for us).

Please let me know what y'all prefer.

xabolcs commented 1 month ago

These two points would be implemented by adding an ".editorconfig" to the root of the project (work like a charm for us).

:+1:

Is .editorconfig able to prevent IntelliJ's IDEA switching "lot of imports" to import *? If not, then it would be nice to update CONTRIBUTING.md with those IDEA settings.

For the latter I use "Class count to use import with '*'" and "Names count to use static import with '*'" settings in IDEA from IntelliJ: Never use wildcard imports

asolntsev commented 1 month ago

I'm personally against wildcard imports

@cies Sorry, I am confused. You dislike wildcard imports, but you've submitted a PR full of wildcard imports?

cies commented 1 month ago

Is .editorconfig able to prevent IntelliJ's IDEA switching "lot of imports" to import *?

Yes. Works for us.

If not, then it would be nice to update CONTRIBUTING.md with those IDEA settings.

Even IDEA settings can be somewhat selectively committed.

cies commented 1 month ago

@cies Sorry, I am confused. You dislike wildcard imports, but you've submitted a PR full of wildcard imports?

@asolntsev Apologies the PR should have been a draft. I saw it had lots of *s, and since I do not like them (except for very special use cases, like when using eDSLs) I wanted to know your ideas before improving the PR.

Shall I fix the PR such that it has:

  1. a .editorconfig
  2. no wildcard imports
  3. no import blocks -- all imports simply i one block follow alphabetical order
asolntsev commented 1 month ago

Shall I fix the PR such that it has:

  1. a .editorconfig
  2. no wildcard imports
  3. no import blocks -- all imports simply i one block follow alphabetical order

Yes, seems so. I personally don't have any strong opinion about wildcard imports. But it's good to fix some style in .editorconfig, so that all developer share it.

cies commented 1 month ago

The formatting rules I suggest here for the imports are in line with the "Google Java Format" rules mentioned in #441.

xabolcs commented 1 month ago

Bruh!

The list of changed files are damn too high! :grinning:

What I found:

I wasn't able to scroll past all the changed files.

cies commented 1 month ago

@xabolcs

in the templating code where source are generated the reformat decreases readability, IMHO

example please? im not aware of any generated code in git

the /* one line comment / doesn't looks good to me

that's Google's way, sure we can tweak it, but i rather have it as vanilla as possible

a few multi-line comments - which doesn't have trailing . or empty comment line - get "onelined" getting worse to read

i have another work lined up to improve readiblity by manually making things look better. (sur a matter of opinion again, but as long as it gets better on the whole it should be good right? or do we have to address every decision with the committee?)

there are a few import xxx.yyy.*; which weren't "extracted" - do you plan "extract" them in another step?

weird, I'll look into this. just to be sure by extracted you mean that the wildcard import is still there?

there are a few place where lot of import xxx.yyy.... was shortened to import xxx.yyy.* - was it intended?

nope, wildcard imports should be gone: good you spotted them, I'll have a look at it.

I wasn't able to scroll past all the changed files.

I did (still overlook things apparently -- pfff)

xabolcs commented 1 month ago

Hi @cies!

Give me a few days to force myself through all the files!

  1. sure, will give you a pointer!
  2. sure, I'm ok with vanilla format.
  3. I thought the same: improve a little bit the comments before (after?) the formatting. Sure will point you to a few!
  4. yep, not just one, but more wildcard import wasn't extracted! Didn't try to check if any was extracted or not.
  5. :ok_hand:
  6. I'll give it a second try!
cies commented 1 month ago

@xabolcs

I found what I missed. The google-java-format Gradle plugin only fixes the code, not the imports, apparently. Those can be by pressing CTRL+ALT+O in IntelliJ while the root folder of the project is selected. The .editorconfig should tell IDEs/editors not to create new wildcard imports and keep m organized in line with the style def. Fixed it!

I thought the same: improve a little bit the comments before (after?) the formatting. Sure will point you to a few!

please. but remember i have a branch locally on which I already cleaned a lot up.

cies commented 1 month ago

@asolntsev @xabolcs I'm okay with merging this into master. Nothing changed really, just some mechanical imports and formatting changes. Tests run fine.

xabolcs commented 1 month ago

I'm okay with merging this into master.

I would like to complete my review session before merging this as its easier to point out the problematic parts.

please. but remember i have a branch locally on which I already cleaned a lot up.

If it's not included in this PR, then I couldn't care less! :stuck_out_tongue_winking_eye: Did you pushed already somewhere to compare with this cleanup-unused-imports branch?

cies commented 1 month ago

I would like to complete my review session before merging this as its easier to point out the problematic parts.

Take your time, git is patient. Lemme know when you are done so we can merge it.

If it's not included in this PR, then I couldn't care less! 😜

It's not. Just so you know before you put your effort in fixing comments and IntelliJ hints: I already took a stab at it.

Did you pushed already somewhere to compare with this cleanup-unused-imports branch?

Nope. I dont dare to. I did all kinds of work in one go, and I know you will not like it. So now I need to cherrypick it into different branches splitting out the different kinds of work in order not to upset you again :smile: (you are right, I'm waaaay to messy with commits, I should train myself)

xabolcs commented 1 month ago

I would like to complete my review session before merging this as its easier to point out the problematic parts.

Sadly, this wasn't done yet. I'll do my homework (and open a PR to discuss) when I have time.