Modules and XDocs: changed setter methods to recieve similar types with field type for easier xdoc validation #3212

Closed
rnveach opened this Issue May 22, 2016 · 3 comments

Comments

Projects
None yet
3 participants
@rnveach
Member

rnveach commented May 22, 2016

From Issue #3187

Affected modules/checks: SuppressWarningsHolder , AtclauseOrderCheck, SingleLineJavadocCheck

To create a JUnit for module's default types we need to know the type we are dealing with to display it correctly. Unfortunately, We use String in the setter for everything, from regular expressions, file locations, to custom enumerations.
It might be better if we could align the setter type to the bean's type, using

private static BeanUtilsBean createBeanUtilsBean() {
.

So we will change bean methods from:

void setProperty(String s) {
    field = s.split(",");
}

into:

void setProperty(String... s) {
    field = s;
}

This may break compatibility in areas where original methods weren't code correctly for all weird issues, like #3250 (comment) where the setter method wasn't trimming spaces from comma delimited input.

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 6, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 6, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 6, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 6, 2016

@rnveach rnveach changed the title from Modules and XDocs: align setter types with field type for easier xdoc option type to Modules and XDocs: changed setter methods to recieve similar types with field type for easier xdoc validation Jun 7, 2016

@romani romani added this to the 7.0 milestone Jun 7, 2016

@romani romani assigned oburn and rnveach and unassigned oburn Jun 7, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 7, 2016

Member

fix is merged.

Member

romani commented Jun 7, 2016

fix is merged.

@romani romani closed this Jun 7, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 7, 2016

Member

@romani What was merged was just for String arrays.
If we want to go all the way for aligning setters so we can fully validate all types for #3187, we need to also do Patterns, enumerations, etc. This issue isn't finished yet.

I counted around 10 different types.

        cub.register(new PatternConverter(), Pattern.class);
        cub.register(new PadOptionConverter(), PadOption.class);
        cub.register(new WrapOptionConverter(), WrapOption.class);
        cub.register(new LineSeparatorConverter(), LineSeparatorOption.class);
        cub.register(new ImportOrderConverter(), ImportOrderOption.class);
        cub.register(new RightCurlyConverter(), RightCurlyOption.class);
        cub.register(new LeftCurlyConverter(), LeftCurlyOption.class);
        cub.register(new ServerityLevelConverter(), SeverityLevel.class);
        cub.register(new BlockOptionConverter(), BlockOption.class);
        cub.register(new ScopeConverter(), Scope.class);
Member

rnveach commented Jun 7, 2016

@romani What was merged was just for String arrays.
If we want to go all the way for aligning setters so we can fully validate all types for #3187, we need to also do Patterns, enumerations, etc. This issue isn't finished yet.

I counted around 10 different types.

        cub.register(new PatternConverter(), Pattern.class);
        cub.register(new PadOptionConverter(), PadOption.class);
        cub.register(new WrapOptionConverter(), WrapOption.class);
        cub.register(new LineSeparatorConverter(), LineSeparatorOption.class);
        cub.register(new ImportOrderConverter(), ImportOrderOption.class);
        cub.register(new RightCurlyConverter(), RightCurlyOption.class);
        cub.register(new LeftCurlyConverter(), LeftCurlyOption.class);
        cub.register(new ServerityLevelConverter(), SeverityLevel.class);
        cub.register(new BlockOptionConverter(), BlockOption.class);
        cub.register(new ScopeConverter(), Scope.class);
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 7, 2016

Member

please split this to separate issue, I need to think about this.
It looks reasonable on first sight, but we need to think about this a long lasting approach.

Member

romani commented Jun 7, 2016

please split this to separate issue, I need to think about this.
It looks reasonable on first sight, but we need to think about this a long lasting approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment