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

join: add support for non-unicode field separators #2902

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

jtracey
Copy link
Contributor

@jtracey jtracey commented Jan 21, 2022

This allows for -t to take invalid unicode (but still single-byte) values
on unix-like platforms. Other platforms, which as of the time of this commit
do not support OsStr::as_bytes(), could possibly be supported in the future,
but would require design decisions as to what that means.

See the section on conversions in the ffi Rust docs for more detail on what is currently exposed.

Lets the 8-bit-t GNU test pass (see #2634).

@jtracey jtracey mentioned this pull request Jan 21, 2022
10 tasks
.arg("non-unicode_2.bin")
.succeeds()
.stdout_only_fixture("non-unicode_sep.expected");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice to update the test on non unix to verify that the error is here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased and pushed with a new test. We have to use Windows for our non-unix, since that's the only other OS that gives any sort of access to the internal OsString representation.

@jtracey jtracey force-pushed the join-non-unicode-sep branch 4 times, most recently from 984a5ba to 04e5538 Compare January 30, 2022 23:41
This allows for `-t` to take invalid unicode (but still single-byte) values
on unix-like platforms. Other platforms, which as of the time of this commit
do not support `OsStr::as_bytes()`, could possibly be supported in the future,
but would require design decisions as to what that means.
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.

3 participants