-
Notifications
You must be signed in to change notification settings - Fork 3
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
Translate messages into french #196
base: main
Are you sure you want to change the base?
Conversation
Thanks @bahadzie for the review |
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
…m main functions instead
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work here. cleanepi will be our first package to provide message in French 🎉
I have left a number of comments but I could not understand all the changes made here. In particular, it is not always clear to me if changes that are not part of the stated scope of the PR & the NEWS.md
bullets are accidents or intended changes. I would strongly recommend you do a self-review of this PR as well to:
- verify you only committed changes you wanted to implement
- all the changes made here are reflected in the NEWS
tr_("Unrecognised column names specified in {.emph rename}."), | ||
i = tr_("Make sure that the columns to be renamed are part of the input data."), # nolint: line_length_linter | ||
i = tr_("To rename columns, use: {.emph rename = c(new_name1 = 'old_name1', new_name2 = 'old_name2')}.") # nolint: line_length_linter | ||
), call = NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call = NULL
is not specified in the other files, is there a specific reason it's needed here?
If I remember correctly, cli is much better than base R at displaying where the error happened so using the default is likely what we want here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to set call = NULL
in main function and use the default behaviour in internal functions. This is because the main functions are documented and explained in details in the vignette.
I will remove this to harmonise with the other messages.
@@ -73,15 +96,24 @@ detect_to_numeric_columns <- function(scan_res) { | |||
values <- values[values > 0L] | |||
values <- values[names(values) != "missing"] | |||
if (setequal(names(values), c("numeric", "character"))) { | |||
if (values[["numeric"]] > (2 * values[["character"]])) { | |||
if (values[["character"]] <= (2 * values[["numeric"]])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes beyond the stated scope of the PR so double checking it is a conscious desired change and not a typo that crept in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realised that the previous expression was making it difficult to send a meaningful note the user. After several twists, I decided to modify the expression and make sure to send a simple and clear message to the user.
Yes, this is beyond the scope of the PR but it was inline with improving the clarity of the messages sent to the user.
Changes in this PR are related to: