-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Enforce file size on Java inputs #11163
Comments
@romani Here is some data-
This includes Input files from both Edit- |
I would prefer a round number like 200. Configuration javadoc, package, imports, class definitions, and the necessary blank lines add up. |
We do not use imports that much, header comment is not that big. Please show me input file that is good to be above 120 lines. There might few only. |
Long files list: https://gist.github.com/pbludov/fe3786436e177939e2041e3596626296 We can start with a limit of 200 lines, and then reduce the limit to 120. |
count of files is not that I worry about. This task will be perfect for introduction to project, so it will be long lasting and done eventually - ok. Having two iterations: to 200 and then to 120 will be a lot of efforts. Contributor will spend same time to split files from xxxxx to 200 as to xxxx to 120. |
I agree that this is a good limit under normal circumstances. Limiting file lengths will help to make violations more clear, and to reduce/ eliminate extension of existing test inputs. In PR for this issue, we should decide on big files to keep with comment explaining why for each suppression, then have contributors simply select suppression with no comment to work on. |
First PR should do suppression "until #11163". If we decide to keep big input, we should make special comment. |
I am ok with 120 |
Here is some more data to help you decide whether it will be 120 or 200.
This number is composed of the following-
Total num of lines before top-level class declaration - 10 (includes the class declaration) |
We come to majority agreement to make it 120. We can always keep in permanent suppression cases that need more for good reason or we can make limit higher later on. @Vyom-Yadav , can you send PR that will do validation and placing all files above limit to temporary suppression ? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
@MANISH-K-07 @Vyom-Yadav now that we have opened an issue on input file/test method naming conventions, please mark your comments as outdated to clean this issue up. |
Bumps [com.google.truth:truth](https://github.com/google/truth) from 1.2.0 to 1.3.0. - [Release notes](https://github.com/google/truth/releases) - [Commits](google/truth@v1.2.0...v1.3.0) --- updated-dependencies: - dependency-name: com.google.truth:truth dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Issue checkstyle#6207: Enale XpathRegressionTest for IlegalTokenText dependency: bump org.pitest:pitest-maven from 1.15.3 to 1.15.4 Bumps [org.pitest:pitest-maven](https://github.com/hcoles/pitest) from 1.15.3 to 1.15.4. - [Release notes](https://github.com/hcoles/pitest/releases) - [Commits](hcoles/pitest@1.15.3...1.15.4) --- updated-dependencies: - dependency-name: org.pitest:pitest-maven dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Issue checkstyle#13213: removed //ok metrics,design Issue checkstyle#13213: Removed //ok from leftout files Issue checkstyle#13999: Resolve pitest suppression for throwAst.getParent() of JavadocMethod Issue checkstyle#13213: Removed //ok noncompilable/naming dependency: bump org.pitest:pitest-maven from 1.15.4 to 1.15.6 Bumps [org.pitest:pitest-maven](https://github.com/hcoles/pitest) from 1.15.4 to 1.15.6. - [Release notes](https://github.com/hcoles/pitest/releases) - [Commits](hcoles/pitest@1.15.4...1.15.6) --- updated-dependencies: - dependency-name: org.pitest:pitest-maven dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Issue checkstyle#11163: Enforced file size inputs Issue checkstyle#6207: Add XPath IT Regression test for CatchParameterName Issue checkstyle#11163: Enforced filesize commentsindentation Issue checkstyle#13213: removed //ok noncompilable/sizes Issue checkstyle#13213: Removed //ok from methodcount Issue checkstyle#11163: enforced filesize MissingJavadocTypeTags Issue checkstyle#6207: Enable XpathRegressiontest for EqualsHashCode Issue checkstyle#13213: Removed //ok from nolinewrap Issue checkstyle#13213: Removed //ok from declarationorder Issue checkstyle#14137: Enable `MockitoStubbing` check Issue checkstyle#13213: removed //ok noncompilable/whitespace Issue checkstyle#13345: Enable StaticVariableNameCheckExamplesTest Issue checkstyle#14137: Enable `TimeZoneUsage` check Issue checkstyle#11163: Enforced filesize TypeNoJavadoc1 doc: release notes for 10.13.0 [maven-release-plugin] prepare release checkstyle-10.13.0 [maven-release-plugin] prepare for next development iteration doc: fix releasenotes.xml line length violation Issue# 13999: RemoveConditionalMutator_EQUAL_IF on if (child.getType() == TokenTypes.PARAMETER_DEF)
From #11156 (review)
All suppressions can be resolved in separate PRs for each Check individually.
Rationale: big input files a hard to review, hard to debug.
All existing inputs are moved to suppression at 3b523bd
checkstyle/config/checkstyle_resources_suppressions.xml
Line 174 in b37ec9c
Now we need to update all that inputs to follow limit 120 and remove related suppressions. Each Check inputs should be in separate Pull Request
Example of update: 8673001
Test method names and input file name should be similar and describe what we are testing. Example from link above:
List of checks for which we need to enforce file size on inputs -
The text was updated successfully, but these errors were encountered: