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

Feature/checkstyle max allowed violations #780

Conversation

alex-arana
Copy link

@alex-arana alex-arana commented Oct 31, 2016

Any of the checked boxes below indicate that I took action:

For all non-trivial changes that modify the behavior or public API:

  • Before submitting a pull request, I started a discussion on the Gradle developer list,
    the forum or can reference a JIRA issue.
  • I considered writing a design document. A design document can be
    brief but explains the use case or problem you are trying to solve,
    touches on the planned implementation approach as well as the test cases
    that verify the behavior. Optimally, design documents should be submitted
    as a separate pull request. Samples
    can be found in the Gradle GitHub repository. Please let us know if you need help with
    creating the design document. We are happy to help!
  • The pull request contains an appropriate level of unit and integration
    test coverage to verify the behavior. Before submitting the pull request
    I ran a build on your local machine via the command
    ./gradlew quickCheck <impacted-subproject>:check.
  • The pull request updates the Gradle documentation like user guide,
    DSL reference and Javadocs where applicable.

alexarana and others added 2 commits November 1, 2016 01:51
… the maximum number of code violations that are tolerated before breaking the build or setting the invoked ANT task failure property.
Update the Contribution Guideline URL in PR template
@jsotuyod
Copy link
Contributor

jsotuyod commented Nov 1, 2016

This is closely related to https://issues.gradle.org/browse/GRADLE-2888 (which is broader, but covers this particular point)

As per that discussion, I would suggest renaming the new property from maxAllowedViolations to maxErrors in order for it to:

  1. match the already existing Checkstyle property, and therefore be more intuitive to Checkstyle users.
  2. avoid confusion between errors and warnings, since all of them are technically violations.

@eriwen eriwen added cla:yes a:feature A new functionality labels Nov 1, 2016
@alex-arana
Copy link
Author

No problem. I have no objections renaming the public property to maxErrors.
I will issue a new pull request to that effect.

After reading GRADLE-2888, it seems some people are interested in exposing
maxWarnings as well. I have no problem adding that later on.

Regards,
Alex

On 1 November 2016 at 23:25, Juan Martín Sotuyo Dodero <
notifications@github.com> wrote:

This is closely related to https://issues.gradle.org/browse/GRADLE-2888
(which is broader, but covers this particular point)

As per that discussion, I would suggest renaming the new property from
maxAllowedViolations to maxErrors in order for it to:

  1. match the already existing Checkstyle property, and therefore be
    more intuitive to Checkstyle users.
  2. avoid confusion between errors and warnings, since all of them are
    technically violations.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#780 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACAnn7prUn9B8LOvJ5MXbc3-cCfPI7E_ks5q5y_TgaJpZM4KlmZ6
.

…' so as to match the already existing Checkstyle property, and therefore be more intuitive to Checkstyle users.
…ets the maximum number of warnings that are tolerated before breaking the build or setting the invoked ANT task failure property.
@jsotuyod
Copy link
Contributor

Awesome that you could add maxWarnings! Bare in mind though, that as is, maxWarnings is not initialized to a default, so it assumes the default for int which is 0, not Integer.MAX_INT.

@alex-arana
Copy link
Author

Good point.

I will set the default value for maxWarnings explicitly to
Integer.MAX_VALUE as documented.

Regards,
Alejandro.

On 22 November 2016 at 02:43, Juan Martín Sotuyo Dodero <
notifications@github.com> wrote:

Awesome that you could add maxWarnings! Bare in mind though, that as is,
maxWarnings is not initialized to a default, so it assumes the default
for int which is 0, not Integer.MAX_INT.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#780 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACAnn-R49b4D9wcDyEk6ryv7udfXLFQfks5rAbw9gaJpZM4KlmZ6
.

Copy link
Contributor

@eriwen eriwen left a comment

Choose a reason for hiding this comment

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

@alex-arana Thank you very much for contributing to Gradle! Changes generally look good, with one required change I will make. I'm going to wait for this to pass our CI pipeline before approving.

*
* @return the maximum number of errors allowed
*/
@Console
Copy link
Contributor

Choose a reason for hiding this comment

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

These properties should be used for UP-TO-DATE checking because they affect the build outcome and so should be annotated @Input

*
* @return the maximum number of warnings allowed
*/
@Console
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as getMaxErrors()

@eriwen
Copy link
Contributor

eriwen commented Dec 14, 2016

@alex-arana This has been merged with e6b5c85..f4b9769 — thank you for contributing!

@eriwen eriwen closed this Dec 14, 2016
eriwen added a commit that referenced this pull request Jan 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants