-
Notifications
You must be signed in to change notification settings - Fork 131
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
checkstyle-tester: Yaml format for project list #866
Comments
@romani please share an example of the format of this yaml |
Also, will the extenion change? |
I detailed the impact of this kind of change at #867 (comment) and #867 (comment). This type of change will be slightly mildly involved. |
Yeah, the issue description really doesn’t capture the large amount of work that will need to be done to support this. |
@nrmancuso , @rnveach , I updated description with more details. |
@romani please show the full document format in valid yaml. Also, we should use yaml/yml file extension |
I placed missed ":" now yaml is valid. |
IMO, project properties should not be array elements. |
Implementation specifics is still being discussed. |
Please share snippet of config that you have in your mind |
yaml structure should be: projects:
- checkstyle:
scm: git
url: https://github.com/checkstyle/checkstyle.git
branch: master
enabled: true
excludes:
- **/.ci-temp/**/*
- **/resources-noncompilable/**/asttreestringprinter/**/*
- **/resources-noncompilable/**/filefilters/**/*
... Override can be provided by: groovy ./diff.groovy --listOfProjects projects-to-test-on.yaml \
--patchConfig checks-nonjavadoc-error.xml -p "$BRANCH" -r ../../.. \
--useShallowClone \
--allowExcludes --mode single -xm "-Dcheckstyle.failsOnError=false" \
--projects.checkstyle.enabled=false |
@nrmancuso , I probably see right now point of confusion. I updated description to show that config will be list of projects, not a single project. |
I am not confused at all, properties for individual projects should not be an array. A “project” is an object and should be treated that way. Yaml is essentially json, if that helps you to conceptualize this idea. |
in this case I do not undersatnd what |
It would override the value of this property. |
and we need |
Sure. We shouldn’t have to have more than one project file anywhere. Default can be disabled, and we enable by override. This way, we can override other properties too, like the reference. |
This is not a way I planned to use this file. If you prolong your idea, you will see that you don't need file, you can provide all fields as arguments. You don't need a file at all. It is good strategy maybe, but it is very separate feature, let's not mix it to this issue proposal. This issue is about to make config multi line. |
Ok, we can use circular dependency on project list in arguments, but in either case, we shouldn’t have a bunch of these files in the config repo; this sort of repetition is indicative of poor design. If anything, we should have exactly one file with all projects, then a simple yaml file with a list of project names for some checks that require special projects, which can be used to compose your list of projects command. We should not have to maintain 50+copies of the same url, etc |
Dependency cost more than copy paste. We just providing an option to not craft in gist something, and reuse ready to use "compiled" configs. This issue is about to make config multi line, let's not put here any future GitHub actions. You might look at it from perspective of our cI jobs, that does disable and enablement of projects. I look at at it from perspective "run on all". So it might be point of misunderstanding. CI execution just need to stop using config, and take config from command line. Sharing same config for two different reasons is not good. We should split . |
We have now very weird format of project list.
https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/projects-to-test-on.properties
It was quick and old decision. It was format for bash language, we deprecated this version a long time ago.
Let's do yaml format for it.
All long lists, should become elements of array in yaml.
Conceptually, we can use any other type that will allow us to make single line in existing format to be multiple lines that easily to manage.
Look at jdk lines to feel a pain of existing format.
existing:
#checkstyle|git|https://github.com/checkstyle/checkstyle.git|master|**/.ci-temp/**/*,**/resources-noncompilable/**/asttreestringprinter/**/*,**/resources-noncompilable/**/filefilters/**/*,**/resources-noncompilable/**/main/**/*,**/resources-noncompilable/**/suppressionsstringprinter/**/*,**/resources-noncompilable/**/gui/**/*,**/resources-noncompilable/**/javadocpropertiesgenerator/**/*,src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/javaparser/InputJavaParser.java,**/InputAllEscapedUnicodeCharacters.java,**/resources-noncompilable/**/javaparser/InputJavaParser.java,**/resources-noncompilable/**/checks/imports/unusedimports/InputUnusedImportsSingleWordPackage.java,**/resources-noncompilable/**/grammar/java19/*,**/resources-noncompilable/**/treewalker/**/*
proposed:
main reason of updaate is exclusion list, that is long, especially i jdk.
usage now:
will be:
we do not need Enable/Disable of projects, we just need list projects as parameter something like
-projects=apache-struts,guava
. No-projects
will mean use all.The text was updated successfully, but these errors were encountered: