Allow adjusting file names of `TextFileBasedViolationStore` #1046

danhaywood · 2023-01-20T10:54:41Z

At the moment it is quite impossible to make any adjustments to the default TextFileBasedViolationStore since it's not part of the public API and hidden within the ViolationStoreFactory.
We now make it public API and allow customizing the way the store creates the rule violation file names so users can adjust the file name to be easier for humans to read.

hankem

Thanks for your contribution!
Note that you'd have to please sign off your commit according to the DCO.

archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationStoreFactory.java

archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java

hankem

I think your're getting close to a build as passing as you can get it! 👏 (Unfortunately, there are issues with the JDK 10 build since recently, and we also need to extend the copyright statement to 2023, but those will be taken care of independently of your PR.)
Oh, speaking of which: I think you'd also need to add this copyright statement to the new file TextFileBasedViolationStore.java... 🙈

archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java

archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationStoreFactory.java

archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java

danhaywood · 2023-01-24T16:57:59Z

I've added a comment to associated issue #1045 to show how I'm using this.

#1045 (comment)

codecholeric · 2023-01-25T07:19:20Z

Hey, thanks a lot for tackling this! Just to pitch in my 2 cents:
Originally I really didn't want to make TextFileBasedViolationStore public API, since for me this was just an implementation detail. But I see, that in some way this has always invited users to tinker with the text files, I think that's just the nature of handling bigger legacy code bases with multiple devs working in parallel (also noticed it e.g. in #1015 (comment) that it really makes it hard to extend the behavior of this ViolationStore). So meanwhile I'm not feeling so strict anymore about keeping this private 😉
That being said, I really don't like inheritance as a way of extending / adjusting logic. Because it feels like a quite tight coupling of API and implementation details and somehow comes back to haunt you at some point. I know that it's convenient to just declare some method as protected and then plug in some logic, but stuff tends to creep in over time. So, if we go down this way and we want to make TextFileBasedViolationStore public, then I would really prefer it to just plug in this name adjusting logic in a different way. E.g. by passing some function like object description -> name into the constructor, or by going some SPI way of defining an interface and dynamically loading the implementation as configured in archunit.properties (the latter seems a little overkill for this case to me).

danhaywood · 2023-01-25T07:41:19Z

Ok, I'll refactor to have a function passed into the constructor.

…

On Wed, 25 Jan 2023, 07:19 Peter Gafert, ***@***.***> wrote: Hey, thanks a lot for tackling this! Just to pitch in my 2 cents: Originally I really didn't want to make TextFileBasedViolationStore public API, since for me this was just an implementation detail. But I see, that in some way this has always invited users to tinker with the text files, I think that's just the nature of handling bigger legacy code bases with multiple devs working in parallel (also noticed it e.g. in #1015 (comment) <#1015 (comment)> that it really makes it hard to extend the behavior of this ViolationStore). So meanwhile I'm not feeling so strict anymore about keeping this private 😉 That being said, I really don't like inheritance as a way of extending / adjusting logic. Because it feels like a quite tight coupling of API and implementation details and somehow comes back to haunt you at some point. I know that it's convenient to just declare some method as protected and then plug in some logic, but stuff tends to creep in over time. So, if we go down this way and we want to make TextFileBasedViolationStore public, then I would really prefer it to just plug in this name adjusting logic in a different way. E.g. by passing some function like object description -> name into the constructor, or by going some SPI way of defining an interface and dynamically loading the implementation as configured in archunit.properties (the latter seems a little overkill for this case to me). — Reply to this email directly, view it on GitHub <#1046 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH33SLKSKT6IIT4FR3AF33WUDHYHANCNFSM6AAAAAAUBML4MU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

codecholeric

Thanks a lot for adjusting this and your continuing effort!! 😃 (And sorry for the slow turnaround, I'm gonna try to be faster now!!)
I added a couple more review comments, let me know what you think. In the very end, I think this should be 2 commits. One with just a simple preparatory refactoring, moving TextFileBasedViolationStore toplevel, making it public and marking it as public API (i.e. change it from implementation detail to public API as logical unit). Then a second one with the extension.
Also, ArchUnit commit messages nowadays all follow the following conventions:

a title with less or equal than 70 chars that is written to complete the sentence "if applied this commit will" (e.g. "(if applied this commit will) make TextFileBasedViolationStore public API")
an empty separator line followed by a background description if necessary (e.g. "While this was originally intended to be an implementation detail, it turned out that users quite often need to adjust details of the TextFileBasedViolationStore. At the moment this is either very challenging or outright impossible, since most programmatic access to the class is quite limited. We now make TextFileBasedViolationStore publicly accessible, which will make it easier to allow decorators or add extension points in the future."

archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java

codecholeric · 2023-02-02T05:48:59Z

archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java

+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.stream.Collectors.toList;
+
+@PublicAPI(usage = INHERITANCE)


This is now usage = ACCESS, right?

I don't think that's correct. We have to configure archunit to tell it to use this new implementation, using freeze.store property, so that means a subclass of TextFileBasedViolationStore.

codecholeric · 2023-02-02T05:49:16Z

archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java

+import static java.util.stream.Collectors.toList;
+
+@PublicAPI(usage = INHERITANCE)
+public class TextFileBasedViolationStore implements ViolationStore {


We can now make the class final 🙂

I don't think that's correct. We have to configure archunit to tell it to use this new implementation, using freeze.store property, so that means a subclass of TextFileBasedViolationStore.

archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java

archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java

codecholeric · 2023-02-02T05:59:43Z

archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java

@@ -119,6 +119,18 @@ public void stores_violations_with_line_breaks() {
        assertThat(violations).as("stored violations").containsExactlyElementsOf(expected);
    }

+    @Test
+    public void class_can_be_overridden_and_rule_file_name_strategy_supplied() {
+        class MySubclassOfTextFileBasedViolationStore extends TextFileBasedViolationStore {


Also this should not be the way to extend it anymore, right? We don't want users to subclass the TextFileBasedViolationStore, but simple create a new instance...

I don't think that's correct. We have to configure archunit to tell it to use this new implementation, using freeze.store property, so that means a subclass of TextFileBasedViolationStore.

danhaywood · 2023-02-05T13:05:20Z

Thanks a lot for adjusting this and your continuing effort!! 😃 (And sorry for the slow turnaround, I'm gonna try to be faster now!!) I added a couple more review comments, let me know what you think. In the very end, I think this should be 2 commits. One with just a simple preparatory refactoring, moving TextFileBasedViolationStore toplevel, making it public and marking it as public API (i.e. change it from implementation detail to public API as logical unit). Then a second one with the extension.

I think there's more than two commits, but to be honest I'm getting towards the end of the effort I'm prepared to work on this --- I have a solution already against 1.0.1 just by copying out all of TextFileBasedViolationStore. I do applaud the high level of coding standards you are applying here, but (and I say this as someone who also works on an open source project at ASF), raising too high a bar can inhibit contributors...

Also, ArchUnit commit messages nowadays all follow the following conventions:

a title with less or equal than 70 chars that is written to complete the sentence "if applied this commit will" (e.g. "(if applied this commit will) make TextFileBasedViolationStore public API")

an empty separator line followed by a background description if necessary (e.g. "While this was originally intended to be an implementation detail, it turned out that users quite often need to adjust details of the TextFileBasedViolationStore. At the moment this is either very challenging or outright impossible, since most programmatic access to the class is quite limited. We now make TextFileBasedViolationStore publicly accessible, which will make it easier to allow decorators or add extension points in the future."

Well, I've done the first, I also use this convention. I think the other info is from the reference to this PR and the related issue.

codecholeric · 2023-02-05T14:51:33Z

Thanks a lot for your efforts, I really appreciate it!! I understand that this is asked a lot from somebody who spends their free time to do a contribution here. So, feel free to call it a day at any point in time!
I don't really want to compromise on code quality though, I would rather try to support you then more so you don't have to invest so much time.
Honestly, I've now also done some changes while you still worked on it after your comment, because I understood you, that you wanted to stop working on it now 🙈 So we now have a clash 😬
I'm just gonna try to incorporate your changes into what I did and then ask you for a review 🙂

codecholeric · 2023-02-05T15:09:16Z

Okay, I solved the merge conflicts I meanwhile got and tried to merge what I had (in parts lengthily) written 🤪 I also added a commit what I mean by delegation. Because I get your point, but I still don't think we need to allow inheriting from TextFileBasedViolationStore. I'd rather provide some default delegate for easy use and have users rely on that. Maybe I'm too anxious here, but I just don't feel good about opening a concrete class with quite some complexity to inheritance as API 😬
Anyway, please check out my commits (sorry that some stuff might look a tiny bit arbitrary now, since we had made very similar changes at the same time and I just kept what I had. I might not have made all the changes like this if I would have reviewed your final state. So if you like some stuff better as it was before, or if I removed something important, please let me know!)

danhaywood · 2023-02-06T09:57:56Z

You changes look good to me, @codecholeric ... I'd love to see this PR merged in now. Thanks.

At the moment it is quite impossible to make any adjustments to the default `TextFileBasedViolationStore`, since the class is fully hidden from the public API and can thus not be instantiated or modified with respect to the implementation (e.g. adjusting the file names of the store). We now make it public API and allow users access to it, so it can be extended through delegation. Signed-off-by: danhaywood <dan@haywood-associates.co.uk> Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>

To make the rule violation files easier to read for humans, we now allow to customize the way the rule violation file names are created, based on the rule description. Signed-off-by: danhaywood <dan@haywood-associates.co.uk> Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>

codecholeric · 2023-02-06T17:53:36Z

Thanks a lot again 😄 As discussed I've refactored/squashed the commits a little to make them easier to comprehend in the future. This way I actually saw that some logging log levels were not the same anymore 🙈 Fixed that on the way.
I'm gonna merge it as soon as the PR is green 👍

danhaywood mentioned this pull request Jan 20, 2023

[suggestion] Allow the mechanism for creating a file name for frozen violations to be specified via an SPI or similar. #1045

Open

hankem reviewed Jan 21, 2023

View reviewed changes

archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationStoreFactory.java Outdated Show resolved Hide resolved

danhaywood force-pushed the main branch from 4d0c574 to 3561f73 Compare January 22, 2023 11:20

hankem reviewed Jan 22, 2023

View reviewed changes

archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java Outdated Show resolved Hide resolved

danhaywood force-pushed the main branch from 24c304f to 44a6248 Compare January 22, 2023 11:30

hankem reviewed Jan 22, 2023

View reviewed changes

danhaywood force-pushed the main branch from f4dc307 to 4c5f1be Compare January 25, 2023 08:35

codecholeric requested changes Feb 2, 2023

View reviewed changes

danhaywood force-pushed the main branch from 43156af to bd58075 Compare February 5, 2023 13:00

danhaywood force-pushed the main branch 2 times, most recently from 0530c38 to 014c442 Compare February 5, 2023 13:29

codecholeric force-pushed the main branch from c6640c0 to e164340 Compare February 5, 2023 15:10

codecholeric changed the title ~~#1045: refactors TextFileBasedViolationStore so that file name ...~~ Allow adjusting file names of TextFileBasedViolationStore Feb 6, 2023

codecholeric and others added 2 commits February 7, 2023 00:43

codecholeric force-pushed the main branch from e164340 to d48e0e1 Compare February 6, 2023 17:51

codecholeric approved these changes Feb 6, 2023

View reviewed changes

codecholeric merged commit 3e01795 into TNG:main Feb 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow adjusting file names of `TextFileBasedViolationStore` #1046

Allow adjusting file names of `TextFileBasedViolationStore` #1046

danhaywood commented Jan 20, 2023 •

edited by codecholeric

Loading

hankem left a comment

hankem left a comment

danhaywood commented Jan 24, 2023

codecholeric commented Jan 25, 2023

danhaywood commented Jan 25, 2023 via email

codecholeric left a comment

codecholeric Feb 2, 2023

danhaywood Feb 2, 2023

codecholeric Feb 2, 2023

danhaywood Feb 2, 2023

codecholeric Feb 2, 2023

danhaywood Feb 2, 2023

danhaywood commented Feb 5, 2023

codecholeric commented Feb 5, 2023 •

edited

Loading

codecholeric commented Feb 5, 2023

danhaywood commented Feb 6, 2023

codecholeric commented Feb 6, 2023

Allow adjusting file names of TextFileBasedViolationStore #1046

Allow adjusting file names of TextFileBasedViolationStore #1046

Conversation

danhaywood commented Jan 20, 2023 • edited by codecholeric Loading

hankem left a comment

Choose a reason for hiding this comment

hankem left a comment

Choose a reason for hiding this comment

danhaywood commented Jan 24, 2023

codecholeric commented Jan 25, 2023

danhaywood commented Jan 25, 2023 via email

codecholeric left a comment

Choose a reason for hiding this comment

codecholeric Feb 2, 2023

Choose a reason for hiding this comment

danhaywood Feb 2, 2023

Choose a reason for hiding this comment

codecholeric Feb 2, 2023

Choose a reason for hiding this comment

danhaywood Feb 2, 2023

Choose a reason for hiding this comment

codecholeric Feb 2, 2023

Choose a reason for hiding this comment

danhaywood Feb 2, 2023

Choose a reason for hiding this comment

danhaywood commented Feb 5, 2023

codecholeric commented Feb 5, 2023 • edited Loading

codecholeric commented Feb 5, 2023

danhaywood commented Feb 6, 2023

codecholeric commented Feb 6, 2023

Allow adjusting file names of `TextFileBasedViolationStore` #1046

Allow adjusting file names of `TextFileBasedViolationStore` #1046

danhaywood commented Jan 20, 2023 •

edited by codecholeric

Loading

codecholeric commented Feb 5, 2023 •

edited

Loading