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

Tooling: Methods order check #1405

Closed
wants to merge 11 commits into from
Closed

Tooling: Methods order check #1405

wants to merge 11 commits into from

Conversation

tgrothe
Copy link
Contributor

@tgrothe tgrothe commented Feb 3, 2024

Draft

fixes #1347

@tgrothe tgrothe self-assigned this Feb 3, 2024
@tgrothe
Copy link
Contributor Author

tgrothe commented Feb 3, 2024

Add order property
Adjust regex pattern
@tgrothe
Copy link
Contributor Author

tgrothe commented Feb 3, 2024

hmm, @cagix könnte das ein Ansatz sein? es wird aber nur geprüft, ob die Reihenfolge stimmt. etwas besseres habe ich bis dato nicht gefunden.

@cagix
Copy link
Member

cagix commented Feb 3, 2024

hmmm. im prinzip könnte das ein schritt in die richtige richtung sein.

aber nur checken und nicht auch entsprechend der konfiguration formatieren können ist mist. da kriegt doch wieder niemand die ide vernünftig konfiguriert...

außerdem bin ich mit checkstyle durch. die haben ihre internen qualitätsprozesse scheinbar nicht wirklich im griff. und eine relativ umfangreiche xml-konfiguration von hand zu pflegen kommt mir auch nicht mehr in die tüte, 2002 ist endgültig vorbei 🤷

@tgrothe
Copy link
Contributor Author

tgrothe commented Feb 3, 2024

hmmm. im prinzip könnte das ein schritt in die richtige richtung sein.

aber nur checken und nicht auch entsprechend der konfiguration formatieren können ist mist. da kriegt doch wieder niemand die ide vernünftig konfiguriert...

außerdem bin ich mit checkstyle durch. die haben ihre internen qualitätsprozesse scheinbar nicht wirklich im griff. und eine relativ umfangreiche xml-konfiguration von hand zu pflegen kommt mir auch nicht mehr in die tüte, 2002 ist endgültig vorbei 🤷

🤣 deshalb ja eigentlich auch nur als zusatz zu Spotless.

die "ordnung" könnten wir einmal mit einer ide wie intellij herstellen...

aber... dann müssten wir eben bei neuen code (zusätzlich) darauf achten, dass checkstyle keine fehlermeldungen ausgibt.

ggf. kann die einbindung in der gradle build auch noch etwas vereinfacht werden.

aber, wenn du sagst nö, dann nö. ;)

@cagix
Copy link
Member

cagix commented Feb 4, 2024

hmmm. im prinzip könnte das ein schritt in die richtige richtung sein.

aber nur checken und nicht auch entsprechend der konfiguration formatieren können ist mist. da kriegt doch wieder niemand die ide vernünftig konfiguriert...

außerdem bin ich mit checkstyle durch. die haben ihre internen qualitätsprozesse scheinbar nicht wirklich im griff. und eine relativ umfangreiche xml-konfiguration von hand zu pflegen kommt mir auch nicht mehr in die tüte, 2002 ist endgültig vorbei 🤷

🤣 deshalb ja eigentlich auch nur als zusatz zu Spotless.

die "ordnung" könnten wir einmal mit einer ide wie intellij herstellen...

das ist aber genau das problem. du hättest jetzt einen linter mit ner konfiguration (checkstyle) und du hättest einen formatter mit ner separaten einstellung (ide).

meine erfahrung ist: das funktioniert so nur auf dem papier. kein mensch kriegt die konfiguration von checkstyle richtig gelesen, geschweige denn das in der ide nachzukonfigurieren.

irgendwas fehlt immer, und dann kannst du nie wieder mergen.

es geht ja nicht darum, das jetzt einmalig zu formatieren. es soll ja auch in zukunft jeder vernünftig formatierten neuen code einchecken...

aber... dann müssten wir eben bei neuen code (zusätzlich) darauf achten, dass checkstyle keine fehlermeldungen ausgibt.

???

ggf. kann die einbindung in der gradle build auch noch etwas vereinfacht werden.

das spielt gar keine rolle. das wird einmal gemacht, aber die einstellungen von checkstyle und ide synchron zu halten wird in der praxis einfach nicht funktionieren.

@cagix
Copy link
Member

cagix commented Feb 4, 2024

prettier ist ein formatierender linter und scheint relativ verbreitet zu sein (js, md). für java gibt es auch ein plugin: https://github.com/jhipster/prettier-java ... scheint sogar checkstyle regeln zu lesen?

bin aber nicht sicher, ob das was taugt.

@tgrothe
Copy link
Contributor Author

tgrothe commented Feb 4, 2024

hm, ja, das "syncen" der einstellungen wird zum problem, weil intellij mehr kann als checkstyle, also müsste man in intellij etwas ändern und in checkstyle auch. zudem prüft checkstyle nur...

ich schaue mal nach dem prettier(-maven)-plugin... das basiert aber auf npm... und es nimmt auch eine checkstyle.xml an. aber den com.github.sevntu.checkstyle.checks.coding.CustomDeclarationOrderCheck wird es nicht kennen, sevntu ist nämlich noch einmal eine erweiterung für checkstyle. 💭

Comment checkstyle out
Comment checkstyle out 2
Use eclipse formatting
@cagix
Copy link
Member

cagix commented Feb 4, 2024

ist auch nur ne idee. ich mag prettier nicht besonders, weil ich normalerweise dafür wieder npm und das ganze javascript-geraffel auf dem rechner brauche. hier scheint es aber ein plugin für maven/gradle zu geben, was das selbst erledigt. aber so richtig klar ist das nicht, dann würde es ja nicht "plugin" heissen .)

und ja, es ist unklar, was es wirklich kann.

@tgrothe
Copy link
Contributor Author

tgrothe commented Feb 4, 2024

das eclipse spotless java plugin kann das vielleicht mit einer eigenen config. ich versuche das gerade herauszufinden.

@tgrothe
Copy link
Contributor Author

tgrothe commented Feb 4, 2024

das eclipse spotless java plugin kann das vielleicht mit einer eigenen config. ich versuche das gerade herauszufinden.

anscheinend geht das doch nicht.

ja, man kann spotless eine eclipse formatter xml angeben. aber in eclipse ist das members sort eine eigene funktion in der ide, unabhängig vom formatter.

siehe auch dieses ticket (leider schon von 2008, vielleicht gibt es auch ein neueres inzwischen): https://bugs.eclipse.org/bugs/show_bug.cgi?id=256717

d.h., selbst wenn wir die formatter einstellungen als xml datei exportieren, gibt es afaik keine option, für members sort order, also das sortieren der methoden.


ich bin inzwischen leider überfragt.

@tgrothe
Copy link
Contributor Author

tgrothe commented Feb 4, 2024

@cagix ich denke, das problem könnte sein, dass das feature "members sort" oder "rearrange members/code" in keinem "offiziellen" style guide (von oracle, google, aosp, ...) ist. also es keine "offiziellen konventionen" dazu gibt.

möglicherweise ist das deshalb ein eigenständiges feature (bzw. funktion) in ij oder eclipse. sozusagen ein "alleinstellungsmerkmal". ;)

ich hatte alle format configs, die hier im spotless source code stehen https://github.com/diffplug/spotless/blob/main/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JavaExtension.java, einmal "durchgeschaut" bzw. ausprobiert, aber da ist keine configuration oder option dabei.

@cagix
Copy link
Member

cagix commented Feb 4, 2024

hmmm. naja, das ticket rennt ja nicht weg. vielleicht findet sich nochmal irgendwas "schönes".

zur allergroßen not kann man ja immer noch checkstyle plus ein regelset in die ci einbinden. dann muss man aber auch ganz klar irgendwo dokumentieren, wie was sein soll und auch wie man seine ide entsprechend konfiguriert. aber genau das möchte ich eigentlich nicht, das bringt eigentlich nur ärger.

@tgrothe
Copy link
Contributor Author

tgrothe commented Feb 5, 2024

@cagix Auch wenn es auf node/npm basiert, war prettier eigentlich eine gute Idee gewesen, es funktioniert aber leider nicht (hab es auch ausprobiert):

https://prettier.io/docs/en/rationale.html#what-prettier-is-_not_-concerned-about

Sorting/moving imports, object keys, class members, JSX keys, CSS properties or anything else. Apart from being a transform rather than just printing (as mentioned above), sorting is potentially unsafe because of side effects (for imports, as an example) and makes it difficult to verify the most important correctness goal.

Edit: https://github.com/diffplug/spotless/tree/main/plugin-gradle#prettier-plugins

@tgrothe tgrothe assigned cagix and unassigned tgrothe and cagix Feb 11, 2024
@tgrothe
Copy link
Contributor Author

tgrothe commented Feb 13, 2024

@cagix Weiß hier erst mal nicht weiter und hab deshalb den assignee entfernt.

@tgrothe tgrothe added the later not now, maybe later label Feb 13, 2024
@cagix
Copy link
Member

cagix commented Feb 17, 2024

@cagix Weiß hier erst mal nicht weiter und hab deshalb den assignee entfernt.

Da hier kein Lösungsvorschlag vorliegt, schließe ich hier.

@cagix cagix closed this Feb 17, 2024
@tgrothe tgrothe deleted the tg/methods_order_1347 branch March 6, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
later not now, maybe later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooling: Formatierung Reihenfolge Methoden
2 participants