-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add a .locale
argument to arrange()
and implement dplyr_locale()
#6263
Merged
DavisVaughan
merged 9 commits into
tidyverse:main
from
DavisVaughan:feature/arrange-locale-2
Jun 13, 2022
Merged
Add a .locale
argument to arrange()
and implement dplyr_locale()
#6263
DavisVaughan
merged 9 commits into
tidyverse:main
from
DavisVaughan:feature/arrange-locale-2
Jun 13, 2022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
This was referenced May 10, 2022
This was referenced May 24, 2022
DavisVaughan
force-pushed
the
feature/arrange-locale-2
branch
from
June 10, 2022 20:35
77c2780
to
6fe7d69
Compare
This allows us to isolate changes in this PR to just `arrange()`, keeping other usage of `vec_order()` the same for now.
Even though these are superseded, this should help ease the transition a little, since without this argument it would be difficult to choose a different locale, and if stringi wasn't installed then you'd unconditionally get a warning you couldn't silence
With the intention being that it shouldn't show up on the pkgdown site reference page. We expect most people to get to this page through `arrange()`.
This is reproducible across all R sessions and OSes, and is much faster, which makes it a good default. It will also make the default of `arrange()` continue to align with what `group_by() + summarize()` returns, since that will also unconditionally use the C locale.
DavisVaughan
force-pushed
the
feature/arrange-locale-2
branch
from
June 10, 2022 20:41
6fe7d69
to
0221c59
Compare
This is a technical detail of `vec_order_radix()` so it is better to keep the conversion close to where we actually call it. Should make calling `arrange_rows()` on its own a little easier too.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supersedes and Closes #5942 (once again. It just got too out of date to be useful.)
Closes #4962
Closes #5090
Part of #5808
This PR adds a
.locale
argument toarrange()
, with 3 possible options:dplyr_locale()
, the default. See below."en_US"
or"fr"
. This requires stringi."C"
, for sorting in the C locale. This does not require stringi.dplyr_locale()
is a new exported helper and returns a string representing the default locale to use. It has the following properties:"C"
dplyr.locale
option is set to a stringi locale identifier or"C"
, that overrides the above default behavior.This is a breaking change, as we no longer respect the
LC_COLLATE
option."1bfa0724-c563-48de-9068-51476e19cf82"
(Update - no changes to worse in revdeps)