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.

Sign up for GitHub

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

Jump to bottom

Allow for moving truncation into the implementing API #68

Merged
merged 6 commits into from
Jan 12, 2021
Merged

Allow for moving truncation into the implementing API #68

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
46ff97c
Allow for moving truncation into the implementing API
mrginglymus Jan 1, 2021
034e114
More options for truncation
mrginglymus Jan 8, 2021
32263c4
Merge branch 'master' into limits
mrginglymus Jan 9, 2021
5fa067d
Consolidate truncated strings into single class
mrginglymus Jan 9, 2021
a320167
Review feedback
mrginglymus Jan 10, 2021
e92aa81
Move trunction text to start when truncating from start, decouple Joi…
mrginglymus Jan 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 0 additions & 63 deletions src/main/java/io/jenkins/plugins/checks/api/ChunkedTruncatedString.java
View file
Open in desktop

This file was deleted.

63 changes: 0 additions & 63 deletions src/main/java/io/jenkins/plugins/checks/api/NewlineTruncatedString.java
View file
Open in desktop

This file was deleted.

82 changes: 54 additions & 28 deletions src/main/java/io/jenkins/plugins/checks/api/TruncatedString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,21 @@
* chunks of complete markdown until an overflow is detected, at which point a message will be added and all future
* additions will be silently discarded.
*/
public abstract class TruncatedString {
public class TruncatedString {

@NonNull
private final List<String> chunks;
@NonNull
private final String truncationText;
private final boolean truncateStart;
private final boolean chunkOnNewlines;


protected TruncatedString(@NonNull final String truncationText, final boolean reverse) {
private TruncatedString(@NonNull final List<String> chunks, @NonNull final String truncationText, final boolean truncateStart, final boolean chunkOnNewlines) {
this.chunks = Collections.unmodifiableList(Objects.requireNonNull(chunks));
this.truncationText = Objects.requireNonNull(truncationText);
this.truncateStart = reverse;
this.truncateStart = truncateStart;
this.chunkOnNewlines = chunkOnNewlines;
}

/**
Expand All @@ -36,7 +42,7 @@ protected TruncatedString(@NonNull final String truncationText, final boolean re
* @return a {@link TruncatedString} wrapping the provided input
*/
static TruncatedString fromString(final String string) {
return new NewlineTruncatedString.Builder().withString(string).build();
return new Builder().setChunkOnNewlines().addText(string).build();
}

/**
Expand All @@ -45,9 +51,16 @@ static TruncatedString fromString(final String string) {
* @return A string comprising the joined chunks.
*/
@Override
public abstract String toString();
public String toString() {
return String.join("", chunks);
}

protected abstract List<String> getChunks();
private List<String> getChunks() {
if (chunkOnNewlines) {
return Arrays.asList(String.join("", chunks).split("(?<=\r?\n)"));
}
return new ArrayList<>(chunks);
}

/**
* Builds the string such that it does not exceed maxSize, including the truncation string.
Expand All @@ -58,39 +71,42 @@ static TruncatedString fromString(final String string) {
*/
@CheckForNull
public String build(final int maxSize) {
List<String> chunks = getChunks();
List<String> parts = getChunks();
if (truncateStart) {
Collections.reverse(chunks);
Collections.reverse(parts);
Copy link
Contributor

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 to truncateStart makes it easier to understand IMO.

Copy link
Contributor Author

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 should

1. Some
2. Very 
3. Long
4. Output
5. Here

Truncated to three lines would come out in reverse come out as

4. Output
5. Here
Output truncated!

or

Output truncated!
4. Output
5. Here

I'm thinking possibly the latter.

}
return chunks.stream().collect(new Joiner(maxSize));
return parts.stream().collect(new Joiner(maxSize));
}


/**
* Base builder for {@link TruncatedString}.
*
* @param <B> the type of {@link TruncatedString} to build
* Builder for {@link TruncatedString}.
*/
public abstract static class Builder<B> {
public static class Builder {
private String truncationText = "Output truncated.";
private boolean truncateStart = false;

protected String getTruncationText() {
return truncationText;
}

protected boolean isTruncateStart() {
return truncateStart;
}

protected abstract B self();
private boolean chunkOnNewlines = false;
private final List<String> chunks = new ArrayList<>();

/**
* Builds the {@link TruncatedString}.
*
* @return the build {@link TruncatedString}.
*/
public abstract TruncatedString build();
public TruncatedString build() {
return new TruncatedString(chunks, truncationText, truncateStart, chunkOnNewlines);
}

/**
* Adds a chunk of text to the buidler.
XiongKezhi marked this conversation as resolved.
Show resolved Hide resolved
*
* @param text the chunk of text to append to this builder
* @return this builder
*/
public Builder addText(@NonNull final String text) {
this.chunks.add(Objects.requireNonNull(text));
return this;
}

/**
* Sets the truncation text.
Expand All @@ -99,19 +115,29 @@ protected boolean isTruncateStart() {
* @return this builder
*/
@SuppressWarnings("HiddenField")
public B withTruncationText(@NonNull final String truncationText) {
public Builder withTruncationText(@NonNull final String truncationText) {
this.truncationText = Objects.requireNonNull(truncationText);
return self();
return this;
}

/**
* Sets truncator to remove excess text from the start, rather than the end.
*
* @return this builder
*/
public B setTruncateStart() {
public Builder setTruncateStart() {
this.truncateStart = true;
return self();
return this;
}

/**
* Sets truncator to chunk on newlines rather than the chunks.
*
* @return this builder
*/
public Builder setChunkOnNewlines() {
this.chunkOnNewlines = true;
return this;
}

}
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/io/jenkins/plugins/checks/ArchitectureTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
class ArchitectureTest {
@ArchTest
static final ArchRule NO_JENKINS_INSTANCE_CALL = PluginArchitectureRules.NO_JENKINS_INSTANCE_CALL;

@ArchTest
static final ArchRule NO_PUBLIC_TEST_CLASSES = PluginArchitectureRules.NO_PUBLIC_TEST_CLASSES;
// Todo: this seems to prevent paramteized non-integration tests.
Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Member

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).

// @ArchTest
// static final ArchRule NO_PUBLIC_TEST_CLASSES = PluginArchitectureRules.NO_PUBLIC_TEST_CLASSES;

@ArchTest
static final ArchRule NO_TEST_API_CALLED = ArchitectureRules.NO_TEST_API_CALLED;
Expand Down
79 changes: 0 additions & 79 deletions src/test/java/io/jenkins/plugins/checks/api/ChunkedTruncatedStringTest.java
View file
Open in desktop

This file was deleted.

Loading