Skip to content

Setup code formatter #248

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

Merged
merged 6 commits into from
Dec 9, 2020

Conversation

s-soroosh
Copy link
Contributor

I setup the default google formatter.
Let me know how you like this format.
@metacosm @adam-sandor @csviri @kirek007

@metacosm
Copy link
Collaborator

metacosm commented Dec 4, 2020

Not a big fan of the Google format but what matters most is consistency so I'm fine with it if people like it. 😄

Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

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

LGTM

@s-soroosh
Copy link
Contributor Author

If we are all fine with it, I'll add a check to the pr.yml to ensure that the code is formatted before being merged into the master branch

@metacosm
Copy link
Collaborator

metacosm commented Dec 7, 2020

If we are all fine with it, I'll add a check to the pr.yml to ensure that the code is formatted before being merged into the master branch

Even better would be to be able to format the code on build or as a pre-commit hook.

@s-soroosh
Copy link
Contributor Author

If we are all fine with it, I'll add a check to the pr.yml to ensure that the code is formatted before being merged into the master branch

Even better would be to be able to format the code on build or as a pre-commit hook.

I've never had a good experience in using pre-commits, they usually make the commits way slow.
Although It's possible to format the code on every build, we would still need the validation to ensure the pushed code changes are actually formatted.

Copy link
Collaborator

@metacosm metacosm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

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

LGTM

Soroosh Sarabadani added 2 commits December 8, 2020 23:11
# Conflicts:
#	operator-framework/src/main/java/io/javaoperatorsdk/operator/ClassMappingProvider.java
#	operator-framework/src/main/java/io/javaoperatorsdk/operator/ControllerUtils.java
#	operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/annotation/AccumulativeMappingWriter.java
#	operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/annotation/ControllerAnnotationProcessor.java
#	operator-framework/src/test/java/io/javaoperatorsdk/operator/ControllerUtilsTest.java
@s-soroosh
Copy link
Contributor Author

Would be good to merge this PR if there is no objection.

@metacosm
Copy link
Collaborator

metacosm commented Dec 8, 2020

@psycho-ir how do we set up our IDEs to use the format automatically?

@csviri
Copy link
Collaborator

csviri commented Dec 9, 2020

@psycho-ir how do we set up our IDEs to use the format automatically?

If you use intelliJ there is a plugin:
https://plugins.jetbrains.com/plugin/8527-google-java-format

@csviri csviri merged commit cfbbac3 into operator-framework:master Dec 9, 2020
@s-soroosh
Copy link
Contributor Author

s-soroosh commented Dec 9, 2020

The google-java-format plugin does not optimize imports with respect to the google format style.
There are XML schemes for both eclipse and Intellij that you can import as code style and no more plugin would be needed.
https://github.com/google/styleguide
We can also add them to the codebase and document in CONTRIBUTING that we follow these styles, wdyt?

@csviri
Copy link
Collaborator

csviri commented Dec 9, 2020

yep that sounds good!

@metacosm
Copy link
Collaborator

metacosm commented Dec 9, 2020

On retrospect, allowing this PR to go through at this point was a huge mistake… I'm in a world of pain to rebase my quite consequent PRs and I fear it's going to take days just to fix everything… 😭

@adam-sandor
Copy link
Collaborator

revert the commits from this PR?

@csviri
Copy link
Collaborator

csviri commented Dec 10, 2020

I think we already merged things after that. We should be more careful next time with these changes :(
I also head conflicts just did not have that much changes. But I also have some code with the new format.

@csviri
Copy link
Collaborator

csviri commented Dec 10, 2020

@metacosm it would not help if you would format your code with the new formatter too on the branch?

@metacosm
Copy link
Collaborator

@csviri that's what I did and I managed to get one branch updated. I need to update the fabric8 client 5.0 branch now… Depending on how it goes, I might just re-apply the changes on top of a new branch 😄

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