Open romani opened 3 months ago
@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 https://github.com/checkstyle/contribution/pull/867#issuecomment-2133719970 and https://github.com/checkstyle/contribution/pull/867#issuecomment-2133724713. 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.
project properties should not be array elements
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.
@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 --projects.checkstyle.enabled=false
is going to do.
in this case I do not undersatnd what
--projects.checkstyle.enabled=false
is going to do.
It would override the value of this property.
and we need --projects.sevntu-checkstyle.enable=false
??? Am I reading you mind properly ?
and we need
--projects.sevntu-checkstyle.enable=false
??? Am I reading you mind properly ?
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.
We shouldn’t have to have more than one project file anywhere.
This is not a way I planned to use this file. I see your idea as separate engagements that we can do later on, it will not be conflicting with my proposal. Right now I don't see where your approach will be useful, but in future I might see.
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
sort of repetition is indicative of poor design.
Dependency cost more than copy paste. In script/application we will have single source, in final result numerous copies to never dependent on anything, just completely independent and fully specified files as we already accustomed to use by gists.
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.