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

Sort default_undesirable docs robustly #2455

Merged
merged 9 commits into from
Dec 16, 2023
Merged

Sort default_undesirable docs robustly #2455

merged 9 commits into from
Dec 16, 2023

Conversation

MichaelChirico
Copy link
Collaborator

Have been getting some spurious diffs vs. main on this. Is anyone else seeing it? I'm a bit surprised because the default* objects are coming from modify_defaults(), which uses platform_independent_order() already:

lintr/R/with.R

Line 59 in fdbb9bf

res <- res[platform_independent_order(names(res))]

Maybe this is a call to improve platform_independent_order()? That's pretty rudimentary indeed:

lintr/R/utils.R

Lines 206 to 207 in fdbb9bf

platform_independent_order <- function(x) order(tolower(gsub("_", "0", x, fixed = TRUE)))
platform_independent_sort <- function(x) x[platform_independent_order(x)]

@IndrajeetPatil
Copy link
Collaborator

Is anyone else seeing it?

Yes, indeed, I was seeing these diffs in this .Rd file while working on some other PRs.

@AshesITR
Copy link
Collaborator

Me too. I manually roll those changes back before committing. Similarly for some snapshot tests (Windows recreates them with CRLF endings on at least one of my machines)

@MichaelChirico
Copy link
Collaborator Author

OK, switched to using LC_COLLATE for full robustness.

Per r-lib/withr#179, it's also possible to see some interference with LC_COLLATE envvar, but not adding that complication yet since this is only needed for dev.

@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4b59aac) 98.53% compared to head (2352a9b) 98.53%.

❗ Current head 2352a9b differs from pull request most recent head f2062ea. Consider uploading reports for the commit f2062ea to get more accurate results

Files Patch % Lines
R/zzz.R 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2455   +/-   ##
=======================================
  Coverage   98.53%   98.53%           
=======================================
  Files         126      126           
  Lines        5676     5676           
=======================================
  Hits         5593     5593           
  Misses         83       83           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Bisaloo
Copy link
Contributor

Bisaloo commented Dec 16, 2023

In case that's useful, method = "radix" will always use C locale for character strings. It can be a convenient way to get locale-independent sorting without directly tweaking the LOCALE settings

@IndrajeetPatil
Copy link
Collaborator

@Bisaloo This is great!

With the default sorting method

callr::r(
  function() withr::with_collate("C", sort(c("a", "_a", "A"))),
  env = c(LC_COLLATE = "C")
)
#> [1] "A"  "_a" "a"

callr::r(
  function() withr::with_collate("en_US", sort(c("a", "_a", "A"))),
  env = c(LC_COLLATE = "en_US")
)
#> [1] "_a" "a"  "A"

With the radix sorting method

callr::r(
  function() withr::with_collate("C", sort(c("a", "_a", "A"), method = "radix")),
  env = c(LC_COLLATE = "C")
)
#> [1] "A"  "_a" "a"

callr::r(
  function() withr::with_collate("en_US", sort(c("a", "_a", "A"), method = "radix")),
  env = c(LC_COLLATE = "en_US")
)
#> [1] "A"  "_a" "a"

Created on 2023-12-16 with reprex v2.0.2

@MichaelChirico
Copy link
Collaborator Author

In case that's useful, method = "radix" will always use C locale for character strings. It can be a convenient way to get locale-independent sorting without directly tweaking the LOCALE settings

awesome idea! thank you {data.table} 😅

@MichaelChirico MichaelChirico merged commit 847d789 into main Dec 16, 2023
21 checks passed
@MichaelChirico MichaelChirico deleted the op-doc-sort branch December 16, 2023 15:28
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.

5 participants