Skip to content

updated CheckStyle config #1243

Closed
Anmol202005 wants to merge 1 commit intoxwiki:masterfrom
Anmol202005:FanOut
Closed

updated CheckStyle config #1243
Anmol202005 wants to merge 1 commit intoxwiki:masterfrom
Anmol202005:FanOut

Conversation

@Anmol202005
Copy link
Copy Markdown

Updated Checkstyle Configuration: ClassFanOutComplexity max value

Summary

This PR updates the checkstyle configuration to increase the ClassFanOutComplexity max property from 20 to 21.

Motivation

A recent build failure was detected in xwiki-platform-livedata-livetable where LiveTableLiveDataConfigurationResolver.java has a ClassFanOutComplexity of 21, exceeding the current max allowed value of 20.

Details

  • Changed the max value from 20 to 21 in the checkstyle configuration
  • This change is needed as a notification that the fixed rule will raise violations after you upgrade to the latest Checkstyle version

Implementation Notes

This PR should be merged after we release the new Checkstyle version and xwiki upgrades to it. Without this change, existing code that was previously compliant may fail validation checks.

Testing Done

Verified that the updated configuration resolves the build error while maintaining code quality standards.

@michitux
Copy link
Copy Markdown
Contributor

I think we would rather solve that with a @SupressWarnings annotation on the affected class than to increase the limit globally, if it shouldn't be possible to change the class to respect the current limit. Do you have any insights why CheckStyle will report a higher fan-out complexity? Which classes are considered that aren't considered right now?

@Anmol202005
Copy link
Copy Markdown
Author

https://github.com/xwiki/xwiki-platform/blob/92df84a3a91bb6dea21adcd35d3ce440757d0a2b/xwiki-platform-core/xwiki-platform-livedata/xwiki-platform-livedata-livetable/src/main/java/org/xwiki/livedata/internal/livetable/LiveTableLiveDataConfigurationResolver.java#L216

.map(Constraint::new).collect(Collectors.toList())

Previously, the check ignored Constraint::new in method references when calculating class fan-out complexity. This was a false negative. Now fixed, the check correctly counts Constraint::new as a dependency, which is why complexity is increased to 21.

@surli
Copy link
Copy Markdown
Member

surli commented Feb 19, 2025

Just for the record I started a proposal in the past to increase the value to 25, thinking that it was default value in checkstyle, see: https://forum.xwiki.org/t/classfanoutcomplexity-metric/9311. We didn't continue the discussion because I was wrong and so we kept the default, and then we added several new exclusions to the count (e.g. https://forum.xwiki.org/t/excludes-initializable-disposable-and-initializationexception-for-classfanoutcomplexity/15479).

Now personally I still have the feeling 20 is often a bit low so we could also restart that discussion. But we need a proposal before performing this kind of change.

@tmortagne
Copy link
Copy Markdown
Member

tmortagne commented Feb 19, 2025

I think we would rather solve that with a @SupressWarnings annotation on the affected class than to increase the limit globally

Yes, this PR is definitely not the right way to deal with this situation. We are not going to change the general rule for this specific case. We could change the general rules for other reason (as mentioned by @surli) but in any case I really doubt the result of the discussion would be 21.

@tmortagne tmortagne self-assigned this Feb 19, 2025
@romani
Copy link
Copy Markdown
Contributor

romani commented Feb 19, 2025

@tmortagne , do you prefer suppression by annotation ? We can change PR to suppress all violations due to this defect fix, to let your team to decided later on better approach.

@tmortagne
Copy link
Copy Markdown
Member

@tmortagne , do you prefer suppression by annotation ? We can change PR to suppress all violations due to this defect fix, to let your team to decided later on better approach.

It seems to me this is what would make the most sense on your side. You are not going to wait for us to endlessly debate on which number we want in the rule :)

@romani
Copy link
Copy Markdown
Contributor

romani commented Feb 19, 2025

@Anmol202005, please change PR to use suppressions.

@Anmol202005
Copy link
Copy Markdown
Author

PR with suppressions raised : xwiki/xwiki-platform#3916

@tmortagne tmortagne closed this Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants