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

Cleanup poms (using same format for all), remove redundant dependencies, plugins (still todo) #1166

Merged
merged 3 commits into from
Jun 6, 2023

Conversation

olamy
Copy link
Contributor

@olamy olamy commented Apr 20, 2023

The goal is to start cleanup of poms:

  • same format for all (4 spaces indent)
  • remove duplicate plugin declaration
  • remove duplicate dependencies
  • ensure a correct pom hierarchy between all projects

@olamy olamy changed the base branch from master to tckrefactor April 20, 2023 10:15
… poms, fix some wrong dependencies not existing

Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
@scottmarlow
Copy link
Contributor

FYI, we have also used 4 spaces indent previously in different files (just looked at some build.xml files on master branch).

I'm not against using a 2 space indent. Do others have input?

@olamy olamy marked this pull request as ready for review April 25, 2023 11:31
@olamy
Copy link
Contributor Author

olamy commented Apr 25, 2023

@scottmarlow the idea is to use similar format for all otherwise it is a bit confusing.

@scottmarlow
Copy link
Contributor

@scottmarlow the idea is to use similar format for all otherwise it is a bit confusing.

Any particular reason why 2 spaces instead of 4 spaces?

@olamy
Copy link
Contributor Author

olamy commented Apr 26, 2023

@scottmarlow the idea is to use similar format for all otherwise it is a bit confusing.

Any particular reason why 2 spaces instead of 4 spaces?

well I guess I put 2 spaces because I'm used to it for all projects I'm working on and because I use a laptop small screen only and I usually prefer condensed for xml.
But agree for sure sounds like a personal choice as any style choice made in our IT world... (spaces vs tabs 🤣 or curly brace on new line etc..) or why green and not yellow 😁

@olamy
Copy link
Contributor Author

olamy commented May 8, 2023

Please could we merge this?
thanks

@scottmarlow
Copy link
Contributor

scottmarlow commented May 9, 2023 via email

@dmatej
Copy link
Contributor

dmatej commented May 10, 2023

I vote NO.

Years ago I used 2 spaces. I learned to use the checkstyle maven plugin to enforce "no tab char" policy in java and xml and some other file types. Since then most projects I was working on use 4 spaces so I "aligned" with that. I would strongly encourage to not being "original" and be just another standard project without "surprises". The only surprise should be simplicity.

2 spaces:

+ shorter lines, important with limit 80 chars on line
- with longer code blocks it makes reading harder

4 space:

+ easier to see the structure even without reading word by word
o in combination with limit 120 chars on line forces the coder to refactor too long and maybe incomprehensible lines

The difference is not so extremely important, but I would say that I see more often fights against 2-space-based conventions while 4-space-based are usually just accepted. That can give some clue too, what is the better option.

@lprimak
Copy link

lprimak commented May 10, 2023

No from me

@ivargrimstad
Copy link
Member

I would stick to 4 spaces. Seems to be the standard used most places

@markt-asf
Copy link
Contributor

I have a personal preference for no (4 spaces) but a much stronger preference for consistency. I can live with either choice.

@joakime
Copy link
Contributor

joakime commented May 10, 2023

Since there's no Eclipse Style Guide for XML files, I think we should lean on the Maven Style Guide (as this PR is only about maven poms)

https://maven.apache.org/developers/conventions/code.html#XML_Code_Style

Which advocates for 2 spaces.

@cesarhernandezgt
Copy link
Contributor

-1 (no) on 2 spaces proposed by this PR.

We should strive for consistency across the rest of the projects under the jakartaee organization... which uses 4 spaces. IDE's can be adjusted to handle profiles in case a particular user/organization/company uses 2 spaces.

@joakime
Copy link
Contributor

joakime commented May 10, 2023

-1 (no) on 2 spaces proposed by this PR.

We should strive for consistency across the rest of the projects under the jakartaee organization... which uses 4 spaces. IDE's can be adjusted to handle profiles in case a particular user/organization/company uses 2 spaces.

Neither Jakarta EE nor the Eclipse Foundation have a Code Style Guide that covers XML style (the scope of this PR).

@OndroMih
Copy link
Contributor

I vote for 4 spaces. I've never worked on a project with 2 space indentation and most IDEs are configured by default to use 4 space indentation, so I perceive that 4 spaces are a de facto standard.

@olamy
Copy link
Contributor Author

olamy commented May 11, 2023

I think there might be some confusion on the scope.
It's only about pom.xml and nothing about Java code style.
Please consider the fact about line length which is impossible with a pom to limit (xml pom will not appreciate a carriage return in the middle of an element). So in some poms with profiles, you can have very long lines...
Note even the Apache Maven team recommends 2 spaces https://maven.apache.org/developers/conventions/code.html#XML_Code_Style.
One of the main used plugin for formatting spotless has a default value of 2 https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#maven-pom
Anyway as you probably have more experience editing poms and it looks there is a consensus here using 4 spaces. I will configure the project to use spotless with 4 spaces.
The original goal was to have a standard/common style for all poms as today there are so many differences...

@olamy
Copy link
Contributor Author

olamy commented May 11, 2023

@lukasj
Copy link
Contributor

lukasj commented May 11, 2023

the only argument I've heard for 2 spaces (or a tab) I can buy is A11Y - disabled person with a screen reader on may be hearing a lot of noise with growing number of spaces. So while I'm for for no (use 4 spaces), the question is - is this project concerned about A11Y?

@OndroMih
Copy link
Contributor

Even if we cared about accessibility, it's a different issue: https://adamtuttle.codes/blog/2021/tabs-vs-spaces-its-an-accessibility-issue/. It's about using Tabs so that people with bad eyesight can increase indent to more spaces in their editor. If we translate it to 2 spaces or 4 spaces, it's clear that 4 spaces are better for such people. I really doubt that a screen reader reads spaces or tabs, they are just blank characters...

@arjantijms
Copy link
Contributor

Very hard NO from me.

For the simple reason that we standardised on a code style years ago.

See https://github.com/eclipse-ee4j/ee4j/blob/master/codestyle/ee4j-eclipse-formatting.xml#L23

@olamy
Copy link
Contributor Author

olamy commented May 12, 2023

no need anymore to vote on 2 vs 4. please read the comment here #1166 (comment)

@scottmarlow
Copy link
Contributor

no need anymore to vote on 2 vs 4. please read the comment here #1166 (comment)

Thanks @olamy for being willing to use 4 space tabs. I think we should wait a bit longer to hear from others as it could go either way still. I haven't counted the votes yet and I think we should wait until May 31 to summarize the counts since that is what I previously suggested.

Thanks,
Scott

@olamy
Copy link
Contributor Author

olamy commented May 16, 2023

no need anymore to vote on 2 vs 4. please read the comment here #1166 (comment)

Thanks @olamy for being willing to use 4 space tabs. I think we should wait a bit longer to hear from others as it could go either way still. I haven't counted the votes yet and I think we should wait until May 31 to summarize the counts since that is what I previously suggested.

Thanks, Scott

no worries it's called democracy or community whatever you prefer :). As said in the mail thread the most important is to have a consensus/commong agreement and the tooling enforcing it and to move forward

@olamy
Copy link
Contributor Author

olamy commented Jun 6, 2023

@scottmarlow it looks as of May 31 the 4 spaces are the winner.
This PR just enforces it and can be merged now.

@scottmarlow scottmarlow merged commit 5d9911b into jakartaee:tckrefactor Jun 6, 2023
@scottmarlow
Copy link
Contributor

Thanks @olamy!

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.

Remove test buckets already moved out of Platform TCK (started by Scott Marlow).