Issue #16378: Validate that all Inputs mentioned all default properties#16760
Conversation
6921ccd to
258daf7
Compare
258daf7 to
6724da2
Compare
|
@pankratz76 will get started with beautification once the CI is green. |
rdiachenko
left a comment
There was a problem hiding this comment.
@Anmol202005 this PR is huge and hard to review properly. Could you think of a way to split it onto smaller ones. Can we focus on updating the inline parser first and applying its validation to a single test only, and after that gradually update all the rest test classes?
6724da2 to
2c89c40
Compare
agreed, what about if we raise a supplemental PR with all changes in input file and update the inline parser in this PR :) |
Let's update inline parser + one test file in this PR. In the follow-up PRs let's do one test package per PR. Please create a list of subtasks in the issue for each package so we can track what has been fixed. |
look a better approach! |
maybe keep them locally in a dedicated branch, so you can reuse later and try to merge after the supplemental PR is in main. |
2c89c40 to
96b15b0
Compare
|
@rdiachenko, |
Thanks, @Anmol202005 . Please make use ci is green, there are failed inspections like this one: <problem>
<file>file://$PROJECT_DIR$/src/test/java/com/puppycrawl/tools/checkstyle/bdd/InlineConfigParser.java</file>
<line>958</line>
<module>project</module>
<package>com.puppycrawl.tools.checkstyle.bdd</package>
<entry_point TYPE="method" FQNAME="com.puppycrawl.tools.checkstyle.bdd.InlineConfigParser void setProperties(com.puppycrawl.tools.checkstyle.bdd.ModuleInputConfiguration.Builder inputConfigBuilder, java.lang.String inputFilePath, java.util.List<java.lang.String> lines, int beginLineNo, java.lang.String moduleName, java.util.Map<java.lang.String,java.lang.String> defaultProperties)"/>
<problem_class id="ParametersPerMethod" severity="ERROR" attribute_key="ERRORS_ATTRIBUTES">Method with too many parameters</problem_class>
<description><code>setProperties()</code> has too many parameters (num parameters = 6) #loc</description>
<highlighted_element>setProperties</highlighted_element>
<language>JAVA</language>
<offset>24</offset>
<length>13</length>
</problem> |
b258725 to
951ddd5
Compare
Pankraz76
left a comment
There was a problem hiding this comment.
nicely done now, try to increase cohesion.
951ddd5 to
d5c538f
Compare
There was a problem hiding this comment.
@Anmol202005 instead of having custom logic to load default properties, could you check if we can use XmlMetaReader.readAllModulesIncludingThirdPartyIfAny() please? From a quick glance, it does what you are trying to achieve and gives you a list of module objects including module properties.
8c283d8 to
0bec7c6
Compare
|
@rdiachenko |
I don't see how it is related to your changes as mutations are in |
51242c1 to
d8cd07f
Compare
Pankraz76
left a comment
There was a problem hiding this comment.
thanks for consistent push, you will get this done.
|
@Anmol202005 , please fix unresovled items above. |
e6d9571 to
998506a
Compare
done. |
romani
left a comment
There was a problem hiding this comment.
I am good with this .
I would like to make some call less nested (to avoid functional mess), but give it to next reviewee to decide
|
I reset review status from Pankraz76, not be blocker for review. |
fixes #16378
updated InlineConfigParser.java to validate that all properties are specified in Input file. if all properties are default, all properties should be mentioned in Input file and all has prefix (default).