Provide a way to store all current violations of a rule and successively only report new ones #181
Conversation
|
DeepCode analyzed this pull request. Click to see more details. |
| * more resilient, e.g. by ignoring the current line number (assume a class has 500 lines, adding a line at the beginning should maybe not affect an | ||
| * unrelated known violation in line 490). | ||
| * <br><br> | ||
| * If you do not configure {@link ViolationStore} or {@link ViolationLineMatcher}, a default will be used (compare the javadoc of the |
There was a problem hiding this comment.
It doesn't seem to me that the javadoc of those classes describes the default implementation used by FreezingArchRule – but I also don't think that it should. Those interfaces should describe their semantics, not examples how they could be implemented.
There was a problem hiding this comment.
Yeah, you seem right, odd 😦 I'm pretty sure I have described that at some point, but I can't find it either. Maybe it was just a draft. But yes, I don't think the default should be documented at the interfaces.
I would document it at FreezingArchRule, because in the end that class decides which default to use. What do you think? I think it's important to document the defaults within the Javadoc.
There was a problem hiding this comment.
I absolutely agree. If you look at my fork, you'll already find a suggestion for that. 😉
There was a problem hiding this comment.
(If you prefer, I could create a PR for my tiny review commits to be added on top of your entire PR.)
There was a problem hiding this comment.
Yes, feel free to create a PR. I would include the configuration of ViolationStore and ViolationLineMatcher via archunit.properties in your adjusted Javadoc of FreezingArchRule as well. What do you think? I think this is currently only documented in the user guide, but I think it should also go into the Javadoc.
I.e. that you can use persistIn(store), but also can configure it via archunit.properties. And likewise for associateViolationLinesVia(matcher).
There was a problem hiding this comment.
I think that documenting the configurability via archunit.properties on the JavaDoc is a very good idea!
I wonder whether one could actually move some JavaDoc from FreezingArchRule to the two factories. They are, admittedly, only implementarion details, but just linking to them for the gory details would make the FreezingArchRule documentation more compact. After all, one can probably use the defaults most of the time.
archunit/src/main/java/com/tngtech/archunit/library/freeze/FreezingArchRule.java
Outdated
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationLineMatcher.java
Outdated
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/library/freeze/StoreReadException.java
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/library/freeze/StoreUpdateFailedException.java
Show resolved
Hide resolved
archunit/src/test/java/com/tngtech/archunit/testutil/assertion/ConditionEventsAssertion.java
Show resolved
Hide resolved
archunit/src/test/java/com/tngtech/archunit/testutil/assertion/ArchRuleCheckAssertion.java
Outdated
Show resolved
Hide resolved
|
|
||
| private CategorizedViolations removeObsoleteViolationsFromStore(EvaluationResult result, List<String> knownViolations) { | ||
| CategorizedViolations categorizedViolations = new CategorizedViolations(matcher, result, knownViolations); | ||
| log.debug("Removing obsolete violations from store: {}", categorizedViolations.getStoredSolvedViolations()); |
There was a problem hiding this comment.
It might be useful to include the number of obsolete violations.
|
|
||
| @Override | ||
| public void save(ArchRule rule, List<String> violations) { | ||
| log.debug("Storing evaluated rule '{}' with violations: {}", rule.getDescription(), violations); |
There was a problem hiding this comment.
Include number of violations?
Same for getViolations below.
There was a problem hiding this comment.
Thumbs up for all your include the number comments 😉
| return replaceCharacter(violations, "\n", "\\\n"); | ||
| } | ||
|
|
||
| // FIXME: Correct word for 'deescaped'? |
There was a problem hiding this comment.
Yes, but the escaped(violations) method should then be escape(violations). Then it's consistent and makes sense, I think.
Review comments to PR #181
DeepCode Report (#8b0b6a)DeepCode analyzed this pull request. |
There was a problem hiding this comment.
I maybe previously didn't make it clear enough that the PR looks very good and that I only had minor comments. I'm looking forward to this feature in the next release of ArchUnit, as I'm sure that it'll be extremely useful for legacy projects to be improved iteratively.
| public void save(ArchRule rule, List<String> violations) { | ||
| log.debug("Storing evaluated rule '{}' with violations: {}", rule.getDescription(), violations); | ||
|
|
||
| UUID ruleId = ensureRuleId(rule); |
There was a problem hiding this comment.
It's not necessary to use UUIDs at this point. This could as well be
| UUID ruleId = ensureRuleId(rule); | |
| String ruleId = ensureRuleFileName(rule); |
where using UUID.randomUUID() could be an implementation detail.
(This would allow one to manipulate stored.rules, renaming a generated UUID to a more desciptive filename.)
I've added the suggested change to my fork, feel free to pull the two commits from there.
There was a problem hiding this comment.
I don't know if I want to encourage people to tinker with the internal structure of the text file based ViolationStore 😉 But I also don't have strong feelings about this...
I any case, I don't understand why you've changed the original debug log that a rule is already stored to an error log? I don't think it's an error case if the rule is already present within the stored properties, because that should be a pretty standard case when existing violations are adjusted or am I mistaken?
There was a problem hiding this comment.
I've switched the log level back to debug, because within the unit test the normal flow of updating violations would now log errors, so I'm pretty sure debug is okay 😉
There was a problem hiding this comment.
Thanks for catching this! I maybe shouldn't try to make contributions late at night before going on vacation...
I didn't intend to commit the changed log level, but only used it to manually (integration-)test the log statement without changing the log configuration in my project. Sorry for not looking at the diff carefully enough!
There was a problem hiding this comment.
No problem, I just thought I've missed something here 😉 No harm done
…y pretty much every implementation will simply add the lines of the violations to the supplied CollectsLines, and every client that is interested in the raw message lines has to write a dummy wrapper around a collection to access the lines. Thus a simple method getDescriptionLines() seems way more reasonable here. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
…le to provide support for legacy applications where this rule will initially be violated many times. Provides two hooks for extension, first a ViolationStore to determine how the given result will be stored. This should enable to pass a simple file based store, but also implement a custom store that persists to a database or a REST endpoint. Second a ViolationLineMatcher to configure how "rough" stored lines of violations should be compared to actual lines of violations on consecutive evaluation of the given rule. This way it is possible to ignore line numbers or details of given messages to make the comparison more robust against refactorings. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
…ies for example for FreezingArchRule as a first client of custom properties. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
…ViolationStore. Also made ViolationStore to use configurable through archunit.properties. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Improved visibility by moving everything (default)factory specific to package private classes. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
…s not exist, instead of throwing an exception. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
…ded multiple lines (like AndConditionEvent) and a new line appeared, all lines would be reported as violation. Now only the new line will be reported. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
…avior where FreezingArchRule would not fail if the count of an already recorded violation with respect to the given matcher was raised. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
…s a massive performance impact (tested on hibernate core where evaluation time was reduced by 50% with this algorithm) Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Signed-off-by: Manfred Hanke <Manfred.Hanke@tngtech.com>
Signed-off-by: Manfred Hanke <Manfred.Hanke@tngtech.com>
…nd ViolationLineMatcher within archunit.properties. Also introduced custom exception for failed initialization of configured ViolationLineMatcher instead of (falsely) throwing a StoreInitializationFailedException. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Signed-off-by: Manfred Hanke <Manfred.Hanke@tngtech.com>
Signed-off-by: Manfred Hanke <Manfred.Hanke@tngtech.com>
…n during the normal flow if an existing stored violation is adjusted. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
codecholeric
force-pushed
the
freeze_rules
branch
from
3eca0ee
to
8b0b6a9
Compare
Provide a way to store all current violations of a rule and successively only report new ones
In bigger projects it is a typical scenario to add a rule that has thousands of violations. Since this is usually too much to fix immediately, this PR adds
FreezingArchRule, a way to "freeze" the state of a rule at the first run and afterwards only report new violations.Additionally
FreezingArchRulewill automatically adjust the stored state if existing violations are solved to avoid regression.