Issue #13758: Fix documentation of pattern array

[Lmh-java]

Issue #13758: Fix documentation of pattern array
Reword documentation for Pattern[] as discussed in #13758 (comment)

[romani]

Ok to merge

[rnveach]

I am not sure why this is split from #14740 . It seems they are inter-connected based on what is decided on the other PR. This change also being in that PR and discussed in this PR's first post seems to align with that.

If minds are changed in the other PR, it will impact this one.

[Lmh-java]

@rnveach Sorry for the confusion. I think the changes in #14740 will be abondoned, based on the discussion #13758 (comment). I need to create a new PR with only documentation updated, and I will close #14740 once I confirm with @romani.

We agree to only update the documentation for now. And leave the update for Pattern[] delimiter for the future, since it will break the backward compatibility, as discussed in #13758 (comment).

[rnveach]

Documentation is still misleading as we don't support Pattern[] in our setter(s). AbstractAutomaticBean only has Pattern and not Pattern[].

[rnveach]

Even if we ignore #14764 (comment) ,

If this updated documentation is correct, please show that all current fake implementations follow it. This area is global documentation, so everything has to follow it otherwise the documentation is wrong.

https://checkstyle.org/checks/imports/importorder.html#Properties

Specify list of type import groups. Every group identified either by a common prefix string, or by a regular expression enclosed in forward slashes (e.g. /regexp/).

Wouldn't this mean this documentation is wrong since every group is a comma? Not really sure what a common prefix string is.

[rnveach]

leave the update for Pattern[] delimiter for the future, since it will break the backward compatibility

Yes suggestion in issue is breaking compatibility. There is no break in #14740 which I see.

[Lmh-java]

If this updated documentation is correct, please show that all current fake implementations follow it

Reply to #14764 (comment):

By searching through GitHub link, I only see 3 checks using Pattern[] right now. Here is a list:

1. ImportOrder
   groups and staticGroups property.

They are separated by commas. However, these two properties are special, since they have a mix of plain string and regex expression. Because of that, these two properties require users to enclose regex expression with /. Plain string represents the so-called "common prefix string", which is the common package name. For example, java.lang.util and java.lang.annotation share the "common prefix string" of "java.lang". Eventually, these plain strings are converted to regex expression as the code shown below.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheck.java

Lines 747 to 777 in e30260d

	private static Pattern[] compilePatterns(String... packageGroups) {
	final Pattern[] patterns = new Pattern[packageGroups.length];
	for (int i = 0; i < packageGroups.length; i++) {
	String pkg = packageGroups[i];
	final Pattern grp;
	// if the pkg name is the wildcard, make it match zero chars
	// from any name, so it will always be used as last resort.
	if (WILDCARD_GROUP_NAME.equals(pkg)) {
	// matches any package
	grp = Pattern.compile("");
	}
	else if (pkg.startsWith(FORWARD_SLASH)) {
	if (!pkg.endsWith(FORWARD_SLASH)) {
	throw new IllegalArgumentException("Invalid group: " + pkg);
	}
	pkg = pkg.substring(1, pkg.length() - 1);
	grp = Pattern.compile(pkg);
	}
	else {
	final StringBuilder pkgBuilder = new StringBuilder(pkg);
	if (!pkg.endsWith(".")) {
	pkgBuilder.append('.');
	}
	grp = Pattern.compile("^" + Pattern.quote(pkgBuilder.toString()));
	}
	patterns[i] = grp;
	}
	return patterns;
	}

IMO, this design is somehow over-engineering, since we can directly tell users to add a ^ and a . to encolse the "common prefix string" to turn the plain string into a regex (also to avoid confusing), instead of mixing regex and plain string together. However, such update will break the compatibility.

3. ClassFanOutComplexity
   excludeClassesRegexps property (inherited from ClassDataAbstractionCouplingCheck).
   This is separated by comma.
4. ClassDataAbstractionCoupling
   excludeClassesRegexps property (inherited from ClassDataAbstractionCouplingCheck).
   This is separated by comma.
   

   checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/metrics/AbstractClassCouplingCheck.java

   Line 159 in e30260d

   public void setExcludeClassesRegexps(String... from) {

[romani]

ImportOrder is using comma separated list, and each element of it additionally special in interpretation, that is described in property description, so it is ok, not ideal design, but ok.

Documentation is still misleading as we don't support Pattern[] in our setter(s). AbstractAutomaticBean only has Pattern and not Pattern[].

We can convert it to String[]. To be more honest with users. Array of patterns is second step of value interpretation in Check.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheck.java

Line 286 in e30260d

public void setGroups(String... packageGroups) {

I am ok to change all such types to String[] and make sure that each property mentioned that all values converted to Regexp.
@rnveach , Do you agree on such update ?

[rnveach]

I think it is better to go with #14740 then confuse users between this being a string or a pattern.

[romani]

then confuse users between this being a string or a pattern.

if property is simple patterns, so Pattern[] is not confusing.
but this Check is not simple Pattern[], it is very special interpretation of Strings, that mostly Pattern for each element.n
If property description explains all detail of type/data interpretation it is all that wee need.
Such types are useful only for UI plugins which can do extra validation of content of property to give validation before config is created+executed.

this is true list of Patterns:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/metrics/AbstractClassCouplingCheck.java

Lines 159 to 163 in e30260d

	public void setExcludeClassesRegexps(String... from) {
	Arrays.stream(from)
	.map(CommonUtil::createPattern)
	.forEach(excludeClassesRegexps::add);
	}

this is not list of Patterns, it is something special, lets keep it String[] and lets desciption of property explain all:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheck.java

Lines 747 to 759 in e30260d

	private static Pattern[] compilePatterns(String... packageGroups) {
	final Pattern[] patterns = new Pattern[packageGroups.length];
	for (int i = 0; i < packageGroups.length; i++) {
	String pkg = packageGroups[i];
	final Pattern grp;
	// if the pkg name is the wildcard, make it match zero chars
	// from any name, so it will always be used as last resort.
	if (WILDCARD_GROUP_NAME.equals(pkg)) {
	// matches any package
	grp = Pattern.compile("");
	}
	else if (pkg.startsWith(FORWARD_SLASH)) {

---

we can merge few updates:

• this PR
• PR with make Pattern[], as first class type - Issue #13758: Add pattern array converter for auto bean #14740 , such type will have design problem when , is used in regexp intentionally. But we can make parsing of array respect escaped \, and not consider it as , (delimiter for new element), in this PR or later in future if somebody ever complain on this.
• convert ImportOrderCheck to String[]

deal ?

[rnveach]

https://github.com/checkstyle/checkstyle/pull/14740/files is a simple pattern. If we need to change only ImportOrderCheck to be String[] type because it is so special, then I am fine with that.

PR with make Pattern[], as first class type - #14740 , such type will have design problem when , is used in regexp intentionally.

This is already a design problem before that PR, so that PR has no negatives and still the positive that we support what we say we do, Pattern[].

[romani]

ok as we agree on update in package, not individually

@Lmh-java , please make such updates in one PR to make it sure that all updates will be done in same time and all see all changes together, they are not big changes, so single PR is fine.

[romani]

lets put all changes to #14740