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 diff text lines missing #95

Merged
merged 4 commits into from
Mar 16, 2023
Merged

Fix diff text lines missing #95

merged 4 commits into from
Mar 16, 2023

Conversation

SimonMarquis
Copy link
Contributor

The \n was added only if the first condition was false because of if/else priorities.

if (a) {
  /**/
} else if (b) {
  /**/
} else {
  /**/
} + x

is in fact executed as

if (a) {
  /**/
} else {
    if (b) {
      /**/
    } else {
      /**/
    } + x
}

Closing #94

The `\n` was added only if the first condition was false because of if/else priorities.
@SimonMarquis
Copy link
Contributor Author

We probably should add tests to avoid future regressions.

}

val diffTextWithPlusAndMinusWithColor: String = diffTextWithPlusAndMinus.lines().joinToString(separator = "") {
if (it.startsWith("-")) {
ColorTerminal.colorify(ColorTerminal.ANSI_RED, it)
val line = if (it.startsWith("-")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would a stringBuilder be good here? You could use .append() and .appendLine() instead of the \n char?

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, it will work as well. I would prefer using the same methods on both colored and non-colored outputs. I'll try to come up with something better.

@handstandsam
Copy link
Collaborator

handstandsam commented Mar 15, 2023

| "We probably should add tests to avoid future regressions."

What isn't covered? I think because we're on 0.5.x and haven't cut it yet, ideally we should be adding tests in now. I felt like 0.4.x ended up being pretty crazy running around trying to fix config cache, so the more tests the better!

--

Thank you for the PR!

@SimonMarquis
Copy link
Contributor Author

The current tests were asserting on the Gradle exception message.

@SimonMarquis
Copy link
Contributor Author

The new version now uses StringBuilder, which makes new lines automatically handled.
Also, I removed the dependency of diffTextWithPlusAndMinusWithColor on diffTextWithPlusAndMinus. They now only rely on diffLines. This makes it more straightforward, as we no longer have to check for "empty" lines and unexpectedly produce a double line break.

@handstandsam
Copy link
Collaborator

Sounds good. Is this ready to merge? Looked fine.

@SimonMarquis
Copy link
Contributor Author

Yup, ready to be merged 👌

@handstandsam handstandsam merged commit a3143bc into dropbox:main Mar 16, 2023
@SimonMarquis SimonMarquis deleted the patch-3 branch March 16, 2023 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants