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

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

Closed
rnveach opened this Issue Jun 7, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Jun 7, 2016

Taken from Issue #3212

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() {
.

This issue is specifically to change all setProperty(String) methods to setProject(Type), where type are patterns or custom enumerations.
I counted around 10 different types that would need to be added.

        cub.register(new PatternConverter(), Pattern.class);
        cub.register(new FileConverter(), File.class);
        cub.register(new UriConverter(), URI.class);
        cub.register(new ServerityLevelConverter(), SeverityLevel.class);
        cub.register(new ScopeConverter(), Scope.class);

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 27, 2016

Member

Yes it is reasonable.

Check are our implementations, they should be able to be created as java beans and used with fully typed parameters. String was used only to simplify customization from XML (strings only format). We already have bunch of converters, we control code that read that XML so we can use custom converters. Users will still use string values in config file to do configuration.

No breaking compatibility is expected.

Member

romani commented Jun 27, 2016

Yes it is reasonable.

Check are our implementations, they should be able to be created as java beans and used with fully typed parameters. String was used only to simplify customization from XML (strings only format). We already have bunch of converters, we control code that read that XML so we can use custom converters. Users will still use string values in config file to do configuration.

No breaking compatibility is expected.

@romani romani added the approved label Jun 27, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 4, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 4, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 4, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 4, 2016

@rnveach rnveach self-assigned this Nov 5, 2016

romani added a commit that referenced this issue Nov 9, 2016

@romani romani added the new feature label Nov 9, 2016

@romani romani added this to the 7.3 milestone Nov 9, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 9, 2016

Member

I did a merge of first big update for RegExp.
But we need to do additional update, in PR , there was a change:

+    public void setFormat(Pattern pattern) {
+        format = pattern.pattern();
+        regexp = pattern;

we do not really need to store format. We need to get rid of this field. We stored it previously as we need to keep unchanged what user gave us. As we changed String to Pattern, there should stay only one field - that store pattern.

Member

romani commented Nov 9, 2016

I did a merge of first big update for RegExp.
But we need to do additional update, in PR , there was a change:

+    public void setFormat(Pattern pattern) {
+        format = pattern.pattern();
+        regexp = pattern;

we do not really need to store format. We need to get rid of this field. We stored it previously as we need to keep unchanged what user gave us. As we changed String to Pattern, there should stay only one field - that store pattern.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 9, 2016

Member

We stored it previously as we need to keep unchanged what user gave us.

@romani To confirm, some checks use format to display violation. We will keep fields for those instances or replace them with pattern.pattern() (which is just a simple getter)?
Examples:


Also to confirm, this will involve renaming regexp to format as we need to find the bean to get its value since we have no getters.

Member

rnveach commented Nov 9, 2016

We stored it previously as we need to keep unchanged what user gave us.

@romani To confirm, some checks use format to display violation. We will keep fields for those instances or replace them with pattern.pattern() (which is just a simple getter)?
Examples:


Also to confirm, this will involve renaming regexp to format as we need to find the bean to get its value since we have no getters.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 9, 2016

Member

@rnveach ,

We will keep fields for those instances or replace them with pattern.pattern() (which is just a simple getter)?

replace with pattern.pattern(), the less fields we have the better.

Also to confirm, this will involve renaming regexp to format as we need to find the bean to get its value since we have no getters.

I do not follow you, can you send me a link to code. If that is not user property - it is ok to rename to keep name correct.

Member

romani commented Nov 9, 2016

@rnveach ,

We will keep fields for those instances or replace them with pattern.pattern() (which is just a simple getter)?

replace with pattern.pattern(), the less fields we have the better.

Also to confirm, this will involve renaming regexp to format as we need to find the bean to get its value since we have no getters.

I do not follow you, can you send me a link to code. If that is not user property - it is ok to rename to keep name correct.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 11, 2016

Member

first post was updated for bean types that need to be moved to referenced issues until API issue can be fixed.

Member

rnveach commented Nov 11, 2016

first post was updated for bean types that need to be moved to referenced issues until API issue can be fixed.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 23, 2016

Member

setters were fixed for mentioned in description Enums.
Other Enums will be covered in #3543 .

Member

romani commented Nov 23, 2016

setters were fixed for mentioned in description Enums.
Other Enums will be covered in #3543 .

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