Skip to content
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

fix line ending problems in FreezingArchRule #516

Merged
merged 2 commits into from
Jan 27, 2021

Conversation

codecholeric
Copy link
Collaborator

@codecholeric codecholeric commented Jan 23, 2021

This addresses two problems:

  1. if the rule description itself had different line separators than it was originally frozen with, TextBasedViolationStore would report contains(rule) == false and thus the result would be stored as new and the rule checking would succeed
  2. if the single lines of the violation descriptions had a different line break than they were originally frozen with, these violations would all be removed as "solved" and the same violations with the different line break would be reported as new.

I think unfortunately the API is a little strange, i.e. that event.getDescriptionLines() would return lines that contain a line break (since what is the use to talk about a list of lines if the lines can contain line breaks and are thus multiple lines in themselves). But unfortunately this is not so easy to change, since the counting of violations is based on this at the moment. For example, that one long cycle report cycle detected ... \n dependencies of ... \n ... is counted as a single "description line". To change this we would have to adjust this mechanism and then optimally make it illegal to return strings with line breaks from event.getDescriptionLines(). But extending ConditionEvent and thus implementing this method is public API, so this might be a breaking change again. Altogether I decided to just fix this within FreezingArchRule for now and see, if there will be further problems with this in other areas.

To solve 1) I have adjusted TextBasedViolationStore to clean up line breaks from property names on save and load. I decided to add it to the file reading direction as well to a) also fix it for stores that have already saved Windows line breaks and b) support manual tinkering adding Windows line breaks in the future. Otherwise just storing them exclusively with \n as line separator would have probably been good enough. It would be better to solve 1) in a store independent way, so other store implementations would not have to deal with this problem, but unfortunately I could not find any way to do this, as long as the API is contains(ArchRule) and store(ArchRule, ...). We could maybe always have converted these rules via contains(rule.as(ensureUnixLineBreaks(rule.getDescription()))), but that also felt a little shady, and might be surprising in a custom store implementation to get rule.as(..) instead of the rule directly.

Problem 2) I have addressed within FreezingArchRule to make it independent of the concrete store implementation. I decided to put adapters on both "ends" of the rule, i.e. on the EvaluationResult producing the violations "lines" on the one side, and the violation store where we read violations from on the other side. That way comparisons should only happen between Unix line separators and violations should always be stored with Unix line breaks within the store.

Resolves: #458
Resolves: #508

@codecholeric codecholeric requested a review from hankem January 23, 2021 17:48
Copy link
Member

@hankem hankem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I've played a bit with violation stores on Windows and Linux and think that it works as intended.

This addresses two problems:
1) if the rule description itself had different line separators than it was originally frozen with, `TextBasedViolationStore` would report `contains(rule) == false` and thus the result would be stored as new and the rule checking would succeed
2) if the single lines of the violation descriptions had a different line break than they were originally frozen with, these violations would all be removed as "solved" and the same violations with the different line break would be reported as new.

I think unfortunately the API is a little strange, i.e. that `event.getDescriptionLines()` would return lines that contain a line break (since what is the use to talk about a list of lines if the lines can contain line breaks and are thus multiple lines in themselves). But unfortunately this is not so easy to change, since the counting of violations is based on this at the moment. For example, that one long cycle report `cycle detected ... \n dependencies of ... \n ...` is counted as a single "description line". To change this we would have to adjust this mechanism and then optimally make it illegal to return strings with line breaks from `event.getDescriptionLines()`. But extending `ConditionEvent` and thus implementing this method is public API, so this might be a breaking change again. Altogether I decided to just fix this within `FreezingArchRule` for now and see, if there will be further problems with this in other areas.

To solve 1) I have adjusted `TextBasedViolationStore` to clean up line breaks from property names on save and load. I decided to add it to the file reading direction as well to a) also fix it for stores that have already saved Windows line breaks and b) support manual tinkering adding Windows line breaks in the future. Otherwise just storing them exclusively with `\n` as line separator would have probably been good enough. It would be better to solve 1) in a store independent way, so other store implementations would not have to deal with this problem, but unfortunately I could not find any way to do this, as long as the API is `contains(ArchRule)` and `store(ArchRule, ...)`. We could maybe always have converted these rules via `contains(rule.as(ensureUnixLineBreaks(rule.getDescription())))`, but that also felt a little shady, and might be surprising in a custom store implementation to get `rule.as(..)` instead of the rule directly.

Problem 2) I have addressed within `FreezingArchRule` to make it independent of the concrete store implementation. I decided to put adapters on both "ends" of the rule, i.e. on the `EvaluationResult` producing the violations "lines" on the one side, and the violation store where we read violations from on the other side. That way comparisons should only happen between Unix line separators and violations should always be stored with Unix line breaks within the store.

Signed-off-by: Peter Gafert <[email protected]>
@codecholeric codecholeric force-pushed the fix-FreezingArchRule-line-separator-problems branch from a49a210 to 762d76d Compare January 27, 2021 21:17
@codecholeric codecholeric merged commit 4db910f into master Jan 27, 2021
@codecholeric codecholeric deleted the fix-FreezingArchRule-line-separator-problems branch January 27, 2021 21:52
codecholeric added a commit that referenced this pull request Feb 21, 2021
This addresses two problems:
1) if the rule description itself had different line separators than it was originally frozen with, `TextBasedViolationStore` would report `contains(rule) == false` and thus the result would be stored as new and the rule checking would succeed
2) if the single lines of the violation descriptions had a different line break than they were originally frozen with, these violations would all be removed as "solved" and the same violations with the different line break would be reported as new.

I think unfortunately the API is a little strange, i.e. that `event.getDescriptionLines()` would return lines that contain a line break (since what is the use to talk about a list of lines if the lines can contain line breaks and are thus multiple lines in themselves). But unfortunately this is not so easy to change, since the counting of violations is based on this at the moment. For example, that one long cycle report `cycle detected ... \n dependencies of ... \n ...` is counted as a single "description line". To change this we would have to adjust this mechanism and then optimally make it illegal to return strings with line breaks from `event.getDescriptionLines()`. But extending `ConditionEvent` and thus implementing this method is public API, so this might be a breaking change again. Altogether I decided to just fix this within `FreezingArchRule` for now and see, if there will be further problems with this in other areas.

To solve 1) I have adjusted `TextBasedViolationStore` to clean up line breaks from property names on save and load. I decided to add it to the file reading direction as well to a) also fix it for stores that have already saved Windows line breaks and b) support manual tinkering adding Windows line breaks in the future. Otherwise just storing them exclusively with `\n` as line separator would have probably been good enough. It would be better to solve 1) in a store independent way, so other store implementations would not have to deal with this problem, but unfortunately I could not find any way to do this, as long as the API is `contains(ArchRule)` and `store(ArchRule, ...)`. We could maybe always have converted these rules via `contains(rule.as(ensureUnixLineBreaks(rule.getDescription())))`, but that also felt a little shady, and might be surprising in a custom store implementation to get `rule.as(..)` instead of the rule directly.

Problem 2) I have addressed within `FreezingArchRule` to make it independent of the concrete store implementation. I decided to put adapters on both "ends" of the rule, i.e. on the `EvaluationResult` producing the violations "lines" on the one side, and the violation store where we read violations from on the other side. That way comparisons should only happen between Unix line separators and violations should always be stored with Unix line breaks within the store.

Resolves: #458
Resolves: #508
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants