Add formatter plugins and automatic format execution profile#41
Add formatter plugins and automatic format execution profile#41xstefank merged 3 commits intojbosstm:mainfrom
Conversation
|
Started testing this pull request with JACOCO profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK17,label=jnlp-agent/121/ |
|
Started testing this pull request with LRA profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK21,label=jnlp-agent/121/ |
|
Started testing this pull request with JACOCO profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK21,label=jnlp-agent/121/ |
|
Started testing this pull request with LRA profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/121/ |
|
JACOCO profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK17,label=jnlp-agent/121/ |
|
LRA profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/121/ |
|
JACOCO profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK21,label=jnlp-agent/121/ |
|
LRA profile tests failed (https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK21,label=jnlp-agent/121/): AS build failed |
I only skimmed the diffs but they look good, thanks for raising it. I'll leave it to Marco to provide the actual review. |
| <configuration> | ||
| <!-- store outside of target to speed up formatting when mvn clean is used --> | ||
| <cachedir>.cache/formatter-maven-plugin-${version.formatter.plugin}</cachedir> | ||
| <configFile>eclipse-format.xml</configFile> |
There was a problem hiding this comment.
I might be "wrong" about where I am going with this - but I noticed this part and wonder if we know that the eclipse format (or maybe rather than a style, it means formating for Eclipse IDE?) is compatible with https://github.com/jbosstm/narayana-checkstyle-config/blob/main/src/main/resources/narayana-checkstyle/checkstyle.xml. Hopefully it is, but we should anyway add an update to the https://github.com/jbosstm/lra/blob/main/CONTRIBUTING.md to describe this formatting requirement too (assuming it is a new one). Like Mike though, I defer to @marcosgopen for the review (but would mention I could imagine sharing this proposed addition to the developer "requirements" on Zulip (for feedback) and merging say a week or so later if there are no blockers raised in the wider community after the chance for feedback but again, I defer to Marco for deciding this.
|
Thanks @xstefank for raising this, I created a zulip topic to talk about this: https://narayana.zulipchat.com/#narrow/channel/323715-developers/topic/Introducing.20a.20new.20formatter.20in.20Narayana.20LRA/with/518074468 |
| <dependencies> | ||
| <dependency> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-ide-config</artifactId> |
There was a problem hiding this comment.
Overall the formatting changes look good, thanks @xstefank
I had a look at the rules (https://github.com/quarkusio/quarkus/blob/main/independent-projects/ide-config/src/main/resources/eclipse-format.xml) and I think they look good.
However I don't actually like to be dependent on a quarkus indipendent-project and I would slightly prefer having our formatting rules in our jbosstm project https://github.com/jbosstm/narayana-checkstyle-config/tree/main. That would mean to open a PR there and make a 'narayana-checkstyle-config' release to be able to customize it. Said that I don't think we need to customize those rules at the moment so we can use the quarkus dependency for the moment. WDYT @tomjenkinson , @jmfinelli and @mmusgrov ?
There was a problem hiding this comment.
That is a great idea, yeah. In my opinion we can decouple it later and not need Martin to change that part now. I do think that we should have the CONTRIBUTING.md reference the expectation (citing a reference to the quarkus one) with this pull request (and we change that later too) so it goes in as close to atomic change as possible but if needed the CONTRIBUTING.md change could be added as a separate commit but we should be ready to go with that.
|
Started testing this pull request with JACOCO profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK21,label=jnlp-agent/122/ |
|
Started testing this pull request with JACOCO profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK17,label=jnlp-agent/122/ |
|
Started testing this pull request with LRA profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK21,label=jnlp-agent/122/ |
|
Started testing this pull request with LRA profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/122/ |
xstefank
force-pushed
the
formatter
branch
2 times, most recently
from
22993d3 to
0954c47
Compare
|
Started testing this pull request with LRA profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK21,label=jnlp-agent/123/ |
|
Started testing this pull request with JACOCO profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK17,label=jnlp-agent/123/ |
|
Started testing this pull request with JACOCO profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK21,label=jnlp-agent/123/ |
|
Started testing this pull request with LRA profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/123/ |
|
LRA profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/122/ |
|
JACOCO profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK21,label=jnlp-agent/123/ |
|
JACOCO profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK17,label=jnlp-agent/123/ |
|
JACOCO profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK17,label=jnlp-agent/122/ |
|
JACOCO profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK21,label=jnlp-agent/122/ |
|
LRA profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/123/ |
|
LRA profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK21,label=jnlp-agent/123/ |
|
LRA profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK21,label=jnlp-agent/122/ |
CONTRIBUTING.md
Outdated
| 1. Perform a Code Review with the project maintainers on the pull request. | ||
|
|
||
| Remember to also run the full build (`./mvnw clean package -DskipTests`) before | ||
| you open the pull request to format your changes. |
There was a problem hiding this comment.
Thank you for pushing this commit in (and the change to the GitHub action - that's very helpful too, thank you). Personally I would prefer the wording to fit more into the list above (in particular I would mention that the way it is parseable at this time doesn't technically direct the contributor to include the formatted code within their pull request and committing takes place in step 2) but in my opinion, updating the CONTRIBUTING.md so as to be able to identify to a contributor that code formatting is a part of the process is the critical thing to introduce with this pull request and that has been done. @marcosgopen is should still provide the review, but I would share that it works for me.
Thank you, Martin!
|
Started testing this pull request with JACOCO profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK17,label=jnlp-agent/124/ |
|
Started testing this pull request with LRA profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK21,label=jnlp-agent/124/ |
|
Started testing this pull request with JACOCO profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK21,label=jnlp-agent/124/ |
|
Started testing this pull request with LRA profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/124/ |
|
JACOCO profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK17,label=jnlp-agent/124/ |
|
LRA profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/124/ |
|
LRA profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK21,label=jnlp-agent/124/ |
.github/workflows/verify-format.yml
Outdated
| @@ -0,0 +1,22 @@ | |||
| name: LRA-CORE | |||
There was a problem hiding this comment.
+1 to have this github action, could you call it 'LRA-FORMATTING' or something like that please? @xstefank
|
Started testing this pull request with LRA profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK21,label=jnlp-agent/125/ |
|
Started testing this pull request with JACOCO profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK17,label=jnlp-agent/125/ |
|
Started testing this pull request with JACOCO profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK21,label=jnlp-agent/125/ |
|
Started testing this pull request with LRA profile: https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/125/ |
|
JACOCO profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK17,label=jnlp-agent/125/ |
|
LRA profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/125/ |
|
JACOCO profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=JACOCO,jdk=openJDK21,label=jnlp-agent/125/ |
|
LRA profile tests passed - Job complete https://jenkins-csb-narayana-ci.dno.corp.redhat.com/job/btny-pulls-lra/PROFILE=LRA,jdk=openJDK21,label=jnlp-agent/125/ |
marcosgopen
left a comment
marcosgopen
left a comment
There was a problem hiding this comment.
Thanks @xstefank for your PR!
|
Thank you @xstefank too - and Marco for the review @marcosgopen maybe we put a |
|
Good point Tom, let's wait for a few days more for community's feedback, I put the on hold label in the meanwhile. |
5 participants
This is using the formatting settings from Quarkus 3.22.2. But we can opt for something custom if there is a need.