Skip to content
This repository has been archived by the owner on Apr 29, 2021. It is now read-only.

Preserve empty output lines in $lines #93

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rfletcher
Copy link

Empty lines in $output are lost when the $lines array is set. That throws off the expected array index of any lines that follow. This patch preserves those empty lines.

Note that the loop was used to support Bash < 4. Otherwise I would have used the new readarray builtin.

@rfletcher
Copy link
Author

Merging this would have the same IFS issue discussed in #90, but I'd hate for you to hold off on a merge for only that reason. If it looks ok otherwise, I could always insert an IFS=$'\n' to preserve the old behavior for now.

@ELLIOTTCABLE
Copy link

ELLIOTTCABLE commented May 10, 2017

Is this waiting on something? Is there anything I can do to help this get merged?

(Edit: Also, this probably deserves a mention in the README — this completely and totally breaks $LINES.)

@rfletcher
Copy link
Author

Is this waiting on something? Is there anything I can do to help this get merged?

Yes and yes. See discussion in #150.

@jasonkarns
Copy link

I believe this has already been resolved with #90 and can be closed?

@rfletcher
Copy link
Author

I'm not familiar with issue #90. I think the easiest way to see if that fixes the problem described here is to run this test with #90 applied: https://github.com/rfletcher/bats/blob/1523459bc7e7d934ea5e90d2dfbd8d7fac7ac1cb/test/bats.bats#L266-L273

If it passes, then great! If the [ "${lines[1]}" = "" ] assertion fails, then there's still a problem preserving empty lines. (And the test is worth adding to the build either way, IMO.)

@jkroepke
Copy link

jkroepke commented Dec 9, 2020

We are hitting the problem on bats master (in combination with bats-assert) today.

This issue still exists

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants