Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow for moving truncation into the implementing API #68
Allow for moving truncation into the implementing API #68
Changes from 4 commits
46ff97c
034e114
32263c4
5fa067d
a320167
e92aa81
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding the truncated string builder in line 66 make sense since it makes things convenient for implementations, but since we already have details URL, I'm not sure the see more link would be necessary for consumers, if not, the truncated string build is unnecessary as well IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#68 (comment)
the see more link was just a helpful extra added in junit it could probably be skipped but truncating is needed afaict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here is a little bit hard to understand at a start (I even left a comment first asking why we want a reversed output), since it requires you to understand the implementation of the joiner first and the join method in reverse acknowledges this special case. So, although the joiner is just an embedded class, adding another field (e.g.
reverse
) and set it identical totruncateStart
makes it easier to understand IMO.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that it's a bit manky. I think part of the problem is that we reverse it in two different places. I think I should be able to refactor it so that the
Joiner
just retuns a plain list of strings and we do the reversal at the end - so the joiner doesn't need to know about reverse; although I'm not sure if that can be done without also moving the truncation text to the start. Would that make more sense? I think it might... so shouldTruncated to three lines would come out in reverse come out as
or
I'm thinking possibly the latter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I know that I'm using a Junit4 style test here, but that's because the parametrized options for Junit5 seem to require a huge amount of repetitive boilerplate for this kind of use-case.
It seems easy enough to fix this rule upstream to permit Junit4 style tests:
/** Junit 5 test classes should not be public. */ public static final ArchRule NO_PUBLIC_TEST_CLASSES = noClasses().that().haveSimpleNameEndingWith("Test") .and().haveSimpleNameNotContaining("_jmh") .and().doNotHaveModifier(JavaModifier.ABSTRACT) + .and().areNotAnnotatedWith(RunWith.class) .and().haveSimpleNameNotEndingWith("ITest") .should().bePublic();
if that's deemed acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uhafner I'd appreciate your input on this, as you're down as the author of these rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with using JUnit 4 in this module. It would be simpler if you would just inline the rule here and adapt it accordingly (or remove it completely).