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

Improve formatting #156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

arjantijms
Copy link
Contributor

No description provided.

@pzygielo
Copy link
Contributor

This change obviously can't be reviewed in this century, but I could re-apply your process and compare results. Can you share the details how it was done?

@arjantijms
Copy link
Contributor Author

Yes, add the Eclipse EE4J formatting file to the Eclipse IDE. Select all packages, choose source -> format.

See https://github.com/eclipse-ee4j/ee4j/tree/master/codestyle

Also, if you set the diff to ignore whitespace it becomes a lot easier to see (but obviously whitespace differences are an important factor of this PR)

@arjantijms
Copy link
Contributor Author

Any progress here?

@pzygielo
Copy link
Contributor

pzygielo commented Oct 9, 2023

I've applied ee4j_intellij_formatting.xml with IDEA and got different result.

Didn't investigate further. Either XMLs are out of sync, or there is no such thing as EE4J rules.

@arjantijms
Copy link
Contributor Author

What if we just say it improved formatting? It's much closer to the formatting of the other EE4J projects this way.

Copy link
Contributor

@dmatej dmatej left a comment

Choose a reason for hiding this comment

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

I agree it is not reviewable, but on the other hand all PRs after that will be - even if IDEA and Eclipse give different results, the difference is not so big (if I remember well).

Yet one thing - there is no such thing as "approved" EE4J rules, approved by all committers. However it can be still used as some rough recommendation - in the future it should be better based on checkstyle plugin, current Eclipse is able to configure it's formatting based on the maven checkstyle plugin config.

@pzygielo
Copy link
Contributor

I let IDEA to modify everything in imported project. That was stupid and 3200 files were changed (including html, properties, etc.) arjantijms/orb@format...pzygielo:orb:ee4j-reformat-idea

But even for java files only - there are differences for spacing, import order, indentations.

I just haven't managed to run Eclipse with the referenced settings (yet).

@pzygielo
Copy link
Contributor

pzygielo commented Oct 12, 2023

So I limited the changes to orbmain only (ee4j-reformat-idea-orbmain) and the results are of course much closer now: https://github.com/arjantijms/orb/compare/format..pzygielo:orb:ee4j-reformat-idea-orbmain.

But this new diff is much more consumable over the single weekend I guess.

@OndroMih
Copy link

I think it's better if there was some script that would do the formatting. Then everybody could run it and compare the results. FOr example, Google formatter (https://github.com/google/google-java-format) is a tool that can executed on command line, although I don't know if it's possible to configure it to match the Eclipse formatting.

I don't like that formatting depends on an IDE. Then we get into debates which IDE is the one to use, because each IDE adds some of its formatting on top of the imported XML config for formatting.

@arjantijms
Copy link
Contributor Author

There is something to say for that indeed. Initially the IntelliJ and Eclipse files were really close, though not identical even then.

Over time each of them added additional rules, with some defaults differing from the other.

That said, even though it can be an exact science, it doesn't have to be. The basics of EE4J formatting is simply old Sun conventions, with 4 spaces for a tab, and a longer line width.

@dmatej
Copy link
Contributor

dmatej commented Oct 13, 2023

Let's not overengineer it. It is the first and probably last large step. I would just check if there weren't for i loops transformed to foreach loops as it is usual trap breaking things which were invisibly already broken. Otherwise formatting should be safe.

@arjantijms
Copy link
Contributor Author

@pzygielo and others, is there some way to resolve this?

The current formatting is horrible, this PR makes it much nicer. There is no perfection, but just a step in the right direction.

@arjantijms arjantijms changed the title Formatting according to EE4J rules Improve formatting Oct 21, 2023
@arjantijms
Copy link
Contributor Author

@pzygielo just a ping. Any progress here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants