Issue #50: Refactored the header recipe to use checkConfiguration #51
Conversation
| final String headerFilePath = config.getProperty(HEADER_FILE_PROPERTY); | ||
| header = Files.readString(Path.of(headerFilePath), charsetToUse); | ||
| } | ||
| } | ||
| catch (CheckstyleException | IOException exception) { | ||
| catch (IOException exception) { |
There was a problem hiding this comment.
Thsi try-catch wraps too much code that doesn't throw exceptions, please include only those code lines that requre this block
| else if (config.getParent().hasProperty(CHARSET_PROPERTY)) { | ||
| charsetToUse = Charset.forName(config | ||
| .getParent().getProperty(CHARSET_PROPERTY)); | ||
| } |
There was a problem hiding this comment.
I think we should make getParent() method private and encapsulate this logic in hasProperty and getProperty. E.g., getProperty internally goes via parents and tryes to find requested property and returns it. This will allow to have simpler contract and easier usage of the config class as the complexity will be hidden.
| else { | ||
| charsetToUse = charset; | ||
| charsetToUse = Charset.defaultCharset(); |
There was a problem hiding this comment.
Does it make sense to add getPropertyOrDefault into config class to handle such cases?
There was a problem hiding this comment.
done. simplified the logic.
| .map(Integer::parseInt) | ||
| .collect(Collectors.toList()); | ||
| } | ||
| if (!config.hasProperty(IGNORE_LINES_PROPERTY)) { |
There was a problem hiding this comment.
Swap this condition with else. E.g., what's easier to read: if (config.hasProperty(...)) or if (!config.hasProperty(...)) ?
|
|
||
| final List<CheckstyleViolation> violations = | ||
| CheckstyleReportParser.parse(Path.of(reportPath)); | ||
|
|
||
| return new Header(violations, | ||
| extractCheckConfiguration(config, "Header"), getCharset(config)); | ||
| final CheckConfiguration checkConfig = config.getChild("Header"); |
There was a problem hiding this comment.
- Could you rename
getChildtogetChildConfig - Using such hardcoded values
"Header"may be error prone, any type or multiple places and we are in trouble. Let's handle this separately, as an option we can create an enum and put there all checks the we have recipes for. Please create and issue for this
There was a problem hiding this comment.
We're already planning to shift to using CheckstyleAutoFix directly, so I don't think we'll end up using CheckEnum.
Anmol202005
force-pushed
the
UpdateHeader
branch
4 times, most recently
from
4d2ba76 to
e6b7f95
Compare
Anmol202005
force-pushed
the
UpdateHeader
branch
from
e6b7f95 to
37b3032
Compare
Part of: #50
simplified the implementation by using checkConfiguration in header.