Skip to content

comm: remaining tests for feature completion #823

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

Merged
merged 1 commit into from
Feb 24, 2016

Conversation

nathanross
Copy link
Contributor

This PR includes

@nathanross nathanross force-pushed the comm-additional-tests branch 2 times, most recently from fb015b4 to 5b10169 Compare February 18, 2016 19:25
@ebfe
Copy link
Contributor

ebfe commented Feb 19, 2016

Not sure if its worth adding locale stuff to the tests right now. It's not like anything is looking at it or am I missing something?

@nathanross
Copy link
Contributor Author

man comm specifies that comm respects LC_COLLATE in regard to determining whether a line is later in sort order from the previous line or not. respect_lc_collate (a test added in the second commit) is an example of this, where when LC_COLLATE=C we expect that uppercase alphabetical characters are considered as before lowercase in sort order, and so a file with a row that line that starts with uppercase that follows a line that starts with lowercase should throw an error during a call like LC_COLLATE=C comm --check-order

LC_COLLATE is overridden by LC_ALL, a commonly set locale variable. I defaulted to explicitly providing the locale of en_US.UTF-8 out of (possibly reflecting my unworldliness / limited experience in these matters) concern for the possibility that someone may run these tests in a locale that uses ascii characters but considers them as having a different sort order than the sort order that was considered when writing the tests.

@ebfe
Copy link
Contributor

ebfe commented Feb 19, 2016

Nothing in uutils or the rust std lib looks at these variables, so it has no effect right now. If we ever add support for locales that's most likely through an external crate, but i don't expect that to happen anytime soon. So until we know how we want to support locales (or at all), i think it makes little sense to add locale specific tests to the utils.

@ebfe
Copy link
Contributor

ebfe commented Feb 19, 2016

cc @Heather @Arcterus for opinions

@cnd
Copy link
Contributor

cnd commented Feb 22, 2016

@ebfe well locale is important sure but I'm also agreed that it should be added globally, that's all I can say

@nathanross nathanross force-pushed the comm-additional-tests branch from 5b10169 to 11f7731 Compare February 22, 2016 14:49
@nathanross
Copy link
Contributor Author

@ebfe @Heather references to locale rm'd.

@nathanross nathanross force-pushed the comm-additional-tests branch from 11f7731 to d1e785c Compare February 22, 2016 14:59
ebfe added a commit that referenced this pull request Feb 24, 2016
comm: remaining tests for feature completion
@ebfe ebfe merged commit 68efb30 into uutils:master Feb 24, 2016
@nathanross nathanross deleted the comm-additional-tests branch July 17, 2016 16:47
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