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

Formatting rules #731

Merged
merged 6 commits into from
Dec 13, 2023
Merged

Formatting rules #731

merged 6 commits into from
Dec 13, 2023

Conversation

manovotn
Copy link
Contributor

@manovotn manovotn commented Dec 9, 2023

Fixes #600

This is a suggestion for formatting rules and import sorting which is:

  • Located in a separate module
    • Can be imported and used in CDI TCKs as well
    • Has no parent currently, otherwise it would create a circular dependency
  • The code and imports are formatted automatically on each build making it friendly for first time contributors
  • CI has setup which skips formatting and instead checks if the sources in PRs are already formatted
  • The set of rules is the same that I've recently added to Weld and which is taken from Quarkus.
    • These are obviously subject for discussion although I guess most things here are matter of preference 🤷
  • Both config files should be importable into IDE

EDIT: I've also created a tracking issue for CDI TCK - jakartaee/cdi-tck#517

ide-config/pom.xml Outdated Show resolved Hide resolved
@Ladicek
Copy link
Contributor

Ladicek commented Dec 11, 2023

Not sure why CI fails, the new Maven module seems to be set up correctly...

Maybe the ide-config module should also inherit from cdi-parent (it will also add license info and SCM info and maybe more)...

@manovotn manovotn force-pushed the formattingRules branch 2 times, most recently from e19f9b5 to d440a17 Compare December 11, 2023 10:16
@manovotn
Copy link
Contributor Author

Both profiles (validate and format) are no longer active by default but instead contain activation condition based on presence of java sources in that given submodule. This avoid triggering the plugin on modules such as parent or spec documentation. It also allows the newly introduced module to declare cdi-parent as its parent and inherit all the configs.

@manovotn manovotn requested a review from Ladicek December 11, 2023 10:22
pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@Azquelt Azquelt left a comment

Choose a reason for hiding this comment

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

I've done a review of the decisions the auto-formatting rules has made.

Mostly they're good, there were only one or two that left me scratching my head.

However, there are quite a few places where the autoformat has wrapped the text just slightly shorter than the existing code and left a paragraph which just has one or two words alone on a line in the middle of it.

This can't be avoided unless you ask the formatter to remove existing newlines that the author added manually. We probably don't want that so these paragraphs should be reflowed manually (all the line breaks removed and then allow the autoformatter to put them in where it wants them).

@manovotn
Copy link
Contributor Author

I've done a review of the decisions the auto-formatting rules has made.

Thanks for the thorough review!

Mostly they're good, there were only one or two that left me scratching my head.

Like I said, I am not against reformulating them, it's just what I've been working with recently and got used to :)

However, there are quite a few places where the autoformat has wrapped the text just slightly shorter than the existing code and left a paragraph which just has one or two words alone on a line in the middle of it.

This can't be avoided unless you ask the formatter to remove existing newlines that the author added manually. We probably don't want that so these paragraphs should be reflowed manually (all the line breaks removed and then allow the autoformatter to put them in where it wants them).

I didn't pay much attention to the re-flows because in HTML view, this shouldn't really make a difference.
Since you took the time to point them all out, would you like to send a PR with suggested changes? Or just push a commit onto this branch if you like.

@Azquelt
Copy link
Member

Azquelt commented Dec 12, 2023

I don't think I can push directly to the branch since I'm not a committer, so I've opened manovotn#1 with my changes.

@manovotn
Copy link
Contributor Author

I don't think I can push directly to the branch since I'm not a committer, so I've opened manovotn#1 with my changes.

Hm, I didn't realize it won't allow you.
Will review the PR tomorrow, thanks a lot!

manovotn and others added 5 commits December 13, 2023 10:46
Break the lines in the InjectionTargetFactory example more
intelligently.

Signed-off-by: Andrew Rouse <anrouse@uk.ibm.com>
Correct the constructor name in the example, which also causes the
formatter to format it correctly.

Signed-off-by: Andrew Rouse <anrouse@uk.ibm.com>
Allow (but don't require) the closing parenthesis of a method invocation
to be on the following line.

Signed-off-by: Andrew Rouse <anrouse@uk.ibm.com>
The autoformat settings will not join lines together which has resulted
in some cases where the line lengths are very uneven because the
formatter has needed to break a line which was slightly too long.

Reflow these paragraphs manually.

Signed-off-by: Andrew Rouse <anrouse@uk.ibm.com>
@manovotn
Copy link
Contributor Author

The PR now contains changes from Andrew and is rebased onto main to resolve conflicts. This also required additional format of files that were meddled with since creation of this PR (which I've squashed into existing formatting commit).
@Ladicek if you're ok with this PR, please approve and we can merge.

@manovotn manovotn merged commit 7bcdd48 into jakartaee:main Dec 13, 2023
4 of 5 checks passed
@manovotn manovotn deleted the formattingRules branch December 13, 2023 10:29
@Ladicek Ladicek added this to the CDI 4.1 milestone Feb 16, 2024
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.

Review unused imports in classes in CDI 4
3 participants