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

New arguments for `ordinal_format` #149

Merged
merged 1 commit into from Jul 9, 2018

Conversation

Projects
None yet
2 participants
@larmarange
Contributor

larmarange commented Jul 7, 2018

prefix, suffix, big.mark and rules (allowing to provide custom rules)

#'
#' # Custom rules for French
#' french <- list(
#' er = "^1$",

This comment has been minimized.

@hadley

hadley Jul 7, 2018

Member

Does this work for 101? In English it would be 101st, but in French 101e?

This comment has been minimized.

@larmarange

larmarange Jul 8, 2018

Contributor

Yes, in French it is "101e" and "1001e", "er" is used only for 1.

#' french <- list(
#' er = "^1$",
#' nd = "^2$",
#' e = "([0-9]+)[12]$",

This comment has been minimized.

@hadley

hadley Jul 7, 2018

Member

What happens if a number matches multiple regular expressions? If it return the first match, we could make the fallbacks (in both French and English) simpler by matching anything (i.e. e = '.')

This comment has been minimized.

@larmarange

larmarange Jul 8, 2018

Contributor

I have done a quick test. If a number match two or more rules, two or more values will be returned. I will have to update the code to consider only the first results

This comment has been minimized.

@larmarange

larmarange Jul 8, 2018

Contributor

corrected

#' @param x A numeric vector of positive values to format.
#' @param prefix,suffix Symbols to display before and after value.
#' @param big.mark Character used between every 3 digits to separate thousands.
#' @param rules Custom rules for computing ordinal indicators (list of regular

This comment has been minimized.

@hadley

hadley Jul 7, 2018

Member

Maybe: Named list of regular expressions, match in order. Name gives suffix, and value specifies which numbers to match.

This comment has been minimized.

@larmarange

larmarange Jul 8, 2018

Contributor

done

stopifnot(all(x > 0))
if (is.null(rules)) {
rules <- list(

This comment has been minimized.

@hadley

hadley Jul 7, 2018

Member

Maybe we should make this ordinal_english() and export it? Then we can use as the default argument, and users can easily see what the default rules are.

This comment has been minimized.

@larmarange

larmarange Jul 8, 2018

Contributor

done

also added ordinal_french() and ordinal_spanish()

@larmarange larmarange force-pushed the larmarange:ordinal_format branch from 0bd819e to ecbb8d7 Jul 8, 2018

@larmarange

This comment has been minimized.

Contributor

larmarange commented Jul 8, 2018

Note: I will resolve conflicts once other PRs merged

@@ -82,6 +82,18 @@ test_that("ordinal format", {
expect_equal(ordinal(21), "21st")

This comment has been minimized.

@hadley

hadley Jul 8, 2018

Member

Can you please add a test for 101st, too?

list(
er = "^1$",
nd = "^2$",
e = "([0-9]+)[12]$",

This comment has been minimized.

@hadley

hadley Jul 8, 2018

Member

Now I think you can simplify this so that e matches anything

This comment has been minimized.

@hadley

hadley Jul 8, 2018

Member

And similarly, English just needs a fall back to th if nothing else matches.

@larmarange larmarange force-pushed the larmarange:ordinal_format branch from ecbb8d7 to e0c3719 Jul 9, 2018

@larmarange

This comment has been minimized.

Contributor

larmarange commented Jul 9, 2018

done, bullet news added and merge conflict solved

New arguments for `ordinal_format`
`prefix`, `suffix`, `big.mark` and `rules` allowing to provide custom rules

@larmarange larmarange force-pushed the larmarange:ordinal_format branch from e0c3719 to 93ae013 Jul 9, 2018

@hadley hadley merged commit fcfb042 into r-lib:master Jul 9, 2018

3 checks passed

codecov/patch 76.47% of diff hit (target 68.11%)
Details
codecov/project 68.29% (+0.18%) compared to a0f0da1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@larmarange larmarange deleted the larmarange:ordinal_format branch Jul 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment