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

Initial import cleanup #22

Merged
merged 4 commits into from Jul 2, 2019
Merged

Initial import cleanup #22

merged 4 commits into from Jul 2, 2019

Conversation

lburgazzoli
Copy link
Contributor

@lburgazzoli lburgazzoli commented Jul 2, 2019

Fixes #14
Fixes #19
Fixes #4

@ppalaga
Copy link
Contributor

ppalaga commented Jul 2, 2019

Is checkstyle already able to fix violations it finds?

@lburgazzoli
Copy link
Contributor Author

It is not yet enabled by default, you can enable it using the sourcecheck profile

@ppalaga
Copy link
Contributor

ppalaga commented Jul 2, 2019

The original setup was more comfortable for the user, because both imports and java source were formatted as necessary and the user did not need to care.

@lburgazzoli
Copy link
Contributor Author

This is to adapt to camel standard, we can improve later but it has to be done cross-project.

@ppalaga
Copy link
Contributor

ppalaga commented Jul 2, 2019

It is not yet enabled by default, you can enable it using the sourcecheck profile

Is the checkstyle plugin just complaining or does it have some sort of a fix mojo that corrects the violations?

@lburgazzoli
Copy link
Contributor Author

it reports the violations as it does on camel

@ppalaga
Copy link
Contributor

ppalaga commented Jul 2, 2019

it reports the violations as it does on camel

No user friendly at all. The original Quarkus setup is better.

@oscerd
Copy link
Contributor

oscerd commented Jul 2, 2019

+1 for having the same configuration we have in Camel.

@ppalaga
Copy link
Contributor

ppalaga commented Jul 2, 2019

I dare to say that "that's how we do it in Camel" does not sound like a reason to do the same thing here unless somebody adds some good explanation "why it was done like that in Camel".

@ppalaga
Copy link
Contributor

ppalaga commented Jul 2, 2019

So could somebody please explain why is the checkstyle plugin better than the autoformatting setup I copied from Quarkus.

I say the Quarkus setup is easier to use, because it fixes all violations it finds and the user does not need to care.

@oscerd
Copy link
Contributor

oscerd commented Jul 2, 2019

First point is to have the same checkstyle we use in Camel.
Second point the camel contributors can have the same approach they have in main repo.
Third point it reports the violations without fails.
Harmonizing the behavior in the Camel subproject make the dev experience better. We need to involve the existing community, after the existing community we can focus on new contributors.

@ppalaga
Copy link
Contributor

ppalaga commented Jul 2, 2019

First point is to have the same checkstyle we use in Camel.
Second point the camel contributors can have the same approach they have in main repo.
Third point it reports the violations without fails.

So there are no plans to enforce the checkstyle checks by making the CI fail if there are violations? I wonder whether people just not going to ignore the checks altogether?

Harmonizing the behavior in the Camel subproject make the dev experience better. We need to involve the existing community, after the existing community we can focus on new contributors.

I do not think this is a valid argument. If in the Camel setup users have to format manually after checkstyle has complained about some violations, with the Quarkus setup, they simply commit the formatted code. They just need to stop caring for the formatting in this repo. They do not need to learn a new process. They just need to forget about the tedious manual steps they needed to perform in Camel.

Please do not insist on keeping bad things from Camel main just because they are from Camel main. These small new subprojects are a great opportunity to try and battle test new things that can be eventually brought back to Camel main.

@oscerd
Copy link
Contributor

oscerd commented Jul 2, 2019

I'm not insisting, I think you are trying to insist. You think what we have in main repo is bad, not me and since you are a new contributor, maybe it would be better to have a bit of humility instead of throwing garbage on the community work and with this, please take me out of this discussion.

<artifactId>license-maven-plugin</artifactId>
<version>${mycila-license.version}</version>
<configuration>
<header>header.txt</header>
Copy link
Contributor

Choose a reason for hiding this comment

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

This setup is going to work only when Maven is called from a directory where license.txt is located. I guess you are aware of that and that you decided to ignore the drawbacks, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is part of the dependency classpath

Copy link
Contributor

Choose a reason for hiding this comment

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

True, thanks for the explanation.

pom.xml Outdated
<exclude>.mvn/**</exclude>
<exclude>mvnw*</exclude>
<exclude>**/META-INF/persistence*.xsd</exclude>
<exclude>**/MyRoutesWithPackage.java</exclude>
Copy link
Contributor

Choose a reason for hiding this comment

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

<exclude>**/pom.xml.versionsBackup</exclude> would be handy in case we'll use versions plugin directly or indirectly via release plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@davsclaus
Copy link
Contributor

@ppalaga it would be appreciated that we keep the drama a bit down at this moment. There is a set of ASF standard and practices that any ASF project must follow, and the project organization and structure we have at Apache Camel - Both Camel K and Camel Quarkus should/must follow that too.

So lets focus on getting Camel Quarkus fully migrated and aligned, and be ready for cutting our first release.

Thanks

<groupId>com.mycila</groupId>
<artifactId>license-maven-plugin</artifactId>
<version>${mycila-license.version}</version>
<configuration>
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to add <failIfUnknown>true</failIfUnknown> or otherwise the plugin will ignore the file types it does not know. See http://code.mycila.com/license-maven-plugin/reports/3.0/check-mojo.html#failIfUnknown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

<artifactId>license-maven-plugin</artifactId>
<version>${mycila-license.version}</version>
<configuration>
<header>header.txt</header>
Copy link
Contributor

Choose a reason for hiding this comment

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

True, thanks for the explanation.

@ppalaga
Copy link
Contributor

ppalaga commented Jul 2, 2019

I am sorry, if my words sounded offending, I was not aiming at hurting anybody. I am new here, striving to understand why you think your traditional setup is better. Please help me understand.

@oscerd oscerd merged commit 1f18332 into apache:master Jul 2, 2019
@lburgazzoli lburgazzoli deleted the cleanup branch July 2, 2019 09:21
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.

Configure license format Configure checkstyle Initial source code - Add license headers
4 participants