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

Some minor tweaks for convert() #217

Merged
merged 6 commits into from
Apr 16, 2024
Merged

Some minor tweaks for convert() #217

merged 6 commits into from
Apr 16, 2024

Conversation

yihui
Copy link
Collaborator

@yihui yihui commented Apr 14, 2024

I'm proposing some minor changes while looking at #216 (these changes are irrelevant to #216, though):

  1. Use requireNamespace() to test if stringi is available.
  2. Reduce one ifelse() call.
  3. Use the safer seq_along(x) instead of 1:length(x).

1. Use `requireNamespace()` to test if stringi is available.
2. Reduce one `ifelse()` call.
3. Use the safer `seq_along(x)` instead of `1:length(x)`.
Copy link
Collaborator Author

@yihui yihui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added more detailed explanations inline.

R/conversion.R Show resolved Hide resolved
R/conversion.R Show resolved Hide resolved
R/conversion.R Outdated
)
unicode_int <- as.integer(as.hexmode(unicode_latex$unicode))
char_latex <- ifelse(unicode_int <= 255 & unicode_int != 177, unicode_latex$chr,
paste0("\\uc1\\u", unicode_int - if (unicode_int < 32768) 0 else 65536, "*")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is another way that I often use by myself, i.e., "ninja code":

Suggested change
paste0("\\uc1\\u", unicode_int - if (unicode_int < 32768) 0 else 65536, "*")
paste0("\\uc1\\u", unicode_int - (unicode_int >= 32768) * 65536, "*")

but I understand that most people don't like obscure ninja code.

R/conversion.R Outdated Show resolved Hide resolved
Co-authored-by: Yihui Xie <[email protected]>
R/conversion.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@nanxstats nanxstats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these change are exactly what we need to improve code quality so I think we should merge this. Thanks for the sprintf() refactor!

(The R CMD check error was due to the secondary dependency of emmeans (estimability) requires R >= 4.3.0 while the current r-oldrel is only 4.2.x. The issue will go away once R 4.4.0 is released.)

@nanxstats nanxstats requested a review from elong0527 April 15, 2024 23:51
@yihui
Copy link
Collaborator Author

yihui commented Apr 16, 2024

(The R CMD check error was due to the secondary dependency of emmeans (estimability) requires R >= 4.3.0 while the current r-oldrel is only 4.2.x. The issue will go away once R 4.4.0 is released.)

If we use emmeans conditionally in the package, it should pass R CMD check in Github actions (with 5302ee7).

@nanxstats nanxstats merged commit dc07990 into Merck:master Apr 16, 2024
7 checks passed
@yihui yihui deleted the patch-1 branch April 16, 2024 18:13
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