Skip to content

Conversation

@kezhenxu94
Copy link
Member

Please answer these questions before submitting pull request

  • Why submit this pull request?

  • Bug fix

  • New feature provided

  • Improve performance

  • Enhancement

  • Related issues

As described in #2871 , we merged a PR that breaks the agent by importing external classes, and a script to check that is provided in #2881 , here I'm proposing another way to check this before our contributors commit and push their codes, by leveraging the checkstyle plugin, if the contributors have a checkstyle plugin installed in their IDE, it would show an error when writing codes, before submitting, which would save some time.

image

@kezhenxu94 kezhenxu94 requested a review from wu-sheng June 19, 2019 04:50
@kezhenxu94 kezhenxu94 added agent Language agent related. bug Something isn't working and you are sure it's a bug! feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. test Test requirements about performance, feature or before release. labels Jun 19, 2019
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Check style module is being removed, right?
And do I still need that scripts to check? Could we remove that?

@wu-sheng wu-sheng added this to the 6.2.0 milestone Jun 19, 2019
@kezhenxu94
Copy link
Member Author

Check style module is being removed, right?

Yes, the module is just used for holding the checkstyle.xml file, and the file can be simply placed in the directory. After removing it, changing the checkstyle.xml doesn't require a deployment of that module.

And do I still need that scripts to check? Could we remove that?

Removing it or not is both ok to me as long as it does no harm

@wu-sheng
Copy link
Member

If you can remove, remove. Because we need consistent configuration in one place. With this PR, it shows in both places.

@kezhenxu94
Copy link
Member Author

If you can remove, remove. Because we need consistent configuration in one place. With this PR, it shows in both places.

done

@dmsolr
Copy link
Member

dmsolr commented Jun 19, 2019

You are so fast @kezhenxu94 . I'm going to do it.

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you and @dmsolr to make this feature better.

@wu-sheng wu-sheng merged commit 4883534 into apache:master Jun 19, 2019
@kezhenxu94 kezhenxu94 deleted the checkstyle branch June 19, 2019 06:11
@kezhenxu94 kezhenxu94 mentioned this pull request Jun 22, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Language agent related. bug Something isn't working and you are sure it's a bug! feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. test Test requirements about performance, feature or before release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants